Skip to content

dolthub/dolt#5861 feat: make dolt_diff_summary respect dolt_ignore patterns #9664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

dolthub/dolt#5861 feat: make dolt_diff_summary respect dolt_ignore patterns #9664

wants to merge 3 commits into from

Conversation

codeaucafe
Copy link
Contributor

@codeaucafe codeaucafe commented Aug 12, 2025 •
edited
Loading

Summary

Makes dolt_diff_summary table function respect dolt_ignore patterns to match dolt diff command behavior.

Changes

  • Added ignore pattern filtering to dolt_diff_summary
  • Tables matching dolt_ignore patterns are now filtered out
  • Added system table filtering for dolt_* prefixed tables
  • Applied filtering to both general and specific table queries

Testing

  • Added 5 integration tests and 4 bats tests

Closes: #5861

@codeaucafe
Copy link
Contributor Author

update: will work on fixing tests accordingly once get some free time from life and work this weekend

@codeaucafe
feat(sql): make dolt_diff_summary respect dolt_ignore patterns
697e09f 
Add ignore pattern filtering to dolt_diff_summary table function to
match dolt diff command behavior. Tables matching patterns in
dolt_ignore are now filtered out from diff summary results.

Changes include:
- Add getIgnorePatternsFromContext() to retrieve ignore patterns from
dolt_ignore
- Add shouldIgnoreDelta() to check if table deltas should be ignored
- Add shouldFilterSystemTable() to filter out system tables (dolt_*
prefix)
- Apply filtering to both general queries and specific table queries
- Only ignore added/dropped tables, not modified/renamed tables

Tests added:
- 5 integration tests in dolt_queries_diff.go
- 4 bats tests in ignore.bats covering basic patterns, wildcards,
dropped tables, and specific table queries

Refs: dolthub#5861
@codeaucafe codeaucafe force-pushed the codeaucafe/refactor/5861-diff-summary-tlb-hide-tables-ignored-by-dolt-ignore branch from 130d807 to 697e09f Compare August 15, 2025 07:13
@codeaucafe
fix(tests): resolve dolt_diff_summary ignore pattern test failures
677c3bd 
Fix test setup issues that were causing failures in integration tests:
- Correct ignore pattern from 'ignored_table' to 'ignored_table2'
- Add initial table creation before commit in three test cases
- Remove "nothing to commit" errors by establishing proper baseline

All dolt_diff_summary ignore functionality tests now pass.

Refs: dolthub#5861
@codeaucafe codeaucafe force-pushed the codeaucafe/refactor/5861-diff-summary-tlb-hide-tables-ignored-by-dolt-ignore branch from 82f8f48 to 677c3bd Compare August 15, 2025 09:44
@codeaucafe
refactor(diff): filter only dolt_ignore table in diff summary
20c3086 
Changed from filtering all dolt_* system tables to only filtering
the dolt_ignore table itself in dolt_diff_summary. This maintains
ignore pattern functionality while being more conservative about
which system tables are filtered, fixing bats test failures.

Refs: dolthub#5861
@codeaucafe
Copy link
Contributor Author

👋 hi, just bumping this up. I think this is a reviewable state. I believe the Bats tests failing are flaky given they all passed locally for me.

thanks in advance

@macneale4 macneale4 self-requested a review September 2, 2025 16:37
@macneale4
Copy link
Contributor

Draft PR to run tests: #9763

macneale4
macneale4 requested changes Sep 2, 2025
View reviewed changes
Copy link
Contributor

@macneale4 macneale4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made all the suggested changes and I'm testing on my own PR. If all that passes, I'll commit to main.

See my changes here: 10d3612

go/libraries/doltcore/sqle/dtablefunctions/dolt_diff_summary.go

// shouldFilterDoltIgnoreTable filters out the dolt_ignore table itself to match dolt diff behavior.
func shouldFilterDoltIgnoreTable(delta diff.TableDelta) bool {
return delta.FromName.Name == "dolt_ignore" || delta.ToName.Name == "dolt_ignore"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return delta.FromName.Name == "dolt_ignore" || delta.ToName.Name == "dolt_ignore"
return delta.FromName.Name == doltdb.IgnoreTableName || delta.ToName.Name == doltdb.IgnoreTableName

go/libraries/doltcore/sqle/dtablefunctions/dolt_diff_summary.go
Comment on lines +468 to +472
_, ok := database.(dsess.SqlDatabase)
if !ok {
return nil, fmt.Errorf("unexpected database type: %T", database)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem necessary. Was there something that came up in testing to indicate this could be a problem?

Since you aren't using the result here, it would seem that you have all the interfaces you need on the sql.Database instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea exactly. I thought I removed this, but it's from when I was debugging 🪦 sorry

go/libraries/doltcore/sqle/dtablefunctions/dolt_diff_summary.go
// can be ignored, not modified/renamed tables.
func shouldIgnoreDelta(delta diff.TableDelta, ignorePatterns doltdb.IgnorePatterns) bool {
// Handle "added" tables (FromName is empty)
if delta.FromName.Name == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delta.IsAdd() here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and delta.IsDrop below

go/libraries/doltcore/sqle/dtablefunctions/dolt_diff_summary.go
Comment on lines +477 to +484
workingRoot, _, _, err := sess.ResolveRootForRef(ctx, dbName, doltdb.Working)
if err != nil {
return nil, err
}

// Create roots object for ignore pattern lookup
roots := doltdb.Roots{Working: workingRoot}
schemas := []string{""} // Default schema
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sess.GetRoots() here.

@macneale4
Copy link
Contributor

I need to get to the bottom of the DoltgreSQL test failure. doesn't seem to be a flake.

@codeaucafe
Copy link
Contributor Author

Thanks @macneale4 i can try and look as well this weekend.

Also, sorry just saw your comments, thanks for fixing your suggestions and sorry about forgetting to remove my debug log call 😆 .

I'll circle back if have questions with failing DoltgreSQL test

@codeaucafe
Copy link
Contributor Author

@macneale4 I looked into the DoltgreSQL GitHub Action failure, but I couldn't pinpoint the actual cause. I'll be out for next couple weeks, so I can look into it further if you don't get to look into it yourself in the meantime.

Once again, thank you for your assistance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@macneale4 macneale4 macneale4 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

contribution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dolt_diff_summary system table should hide tables ignored by dolt_ignore

3 participants

@codeaucafe @macneale4 @coffeegoddd