#5861 refactor: make dolt_diff_summary respect dolt_ignore patterns #9946
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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
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
Update shouldFilterDoltIgnoreTable to use strings.EqualFold instead of case-sensitive equality comparison when filtering dolt_ignore tables from diff summary results. In DoltgreSQL (PostgreSQL mode), table names may have different case representations internally. The case-sensitive comparison (==) was failing to properly filter out dolt_ignore tables, causing them to appear in dolt_diff_summary results when they should be excluded. Using strings.EqualFold ensures the filter works consistently regardless of case variations, matching the behavior of other table name comparisons in the codebase (e.g., findMatchingDelta). This fixes DoltgreSQL integration test failures in TestUserSpaceDoltTables/dolt_ignore test cases. Refs: dolthub#5861
Update dolt_diff_summary to properly filter dolt_ignore table in DoltgreSQL by handling schema-qualified table names (e.g., "public.dolt_ignore"). The previous implementation only checked for exact matches against "dolt_ignore", which failed when table names included schema prefixes. Changes: - Add isIgnoreTable helper function to check both simple and qualified table names - Extract base table name from schema-qualified names using LastIndex for dot separator - Maintain backward compatibility with unqualified names This should fix the DoltgreSQL integration test failures where dolt_ignore tables were incorrectly appearing in diff summaries. Refs: dolthub#5861
Remove dolt_ignore auto-filter when user explicitly requests the table by name. Previously, querying dolt_diff_summary with dolt_ignore as the table parameter would incorrectly return empty results even though the table was explicitly requested. The dolt_ignore table is still auto-filtered from general listings (when no table name is provided), but can now be queried directly when specified by name, matching expected behavior where users can query any table explicitly. This fixes DoltgreSQL integration test failures where explicit dolt_ignore queries were being blocked, something I missed in my implementation originally. Refs: dolthub#5861
Remove the shouldFilterDoltIgnoreTable function and its usage, which was filtering out the dolt_ignore table itself from diff results in dolt_diff_summary. From my understanding of code, dolt_ignore table should appear in diff summaries just like any other table. Only tables that match patterns defined IN the dolt_ignore table should be filtered from results, which is correctly handled by the shouldIgnoreDelta function. This is an attempts to fix the DoltgreSQL integration test failure in TestUserSpaceDoltTables/dolt_ignore which expects dolt_ignore table changes to be visible in diff_summary results. Refs: dolthub#5861
Update test expectations to correctly reflect that the dolt_ignore table itself should appear in dolt_diff_summary results when modified. Fixed four tests in dolt_queries_diff.go: - basic ignore functionality: expect dolt_ignore + filtered tables - wildcard patterns: expect dolt_ignore + non-matching tables - mixed patterns: expect dolt_ignore + explicitly included tables - dropped tables: expect dolt_ignore as "modified" when updated These tests were originally written with incorrect expectations that dolt_ignore would be filtered from results. The DoltgreSQL integration tests had the correct expectations all along. Refs: dolthub#5861
Fix the "dropped tables" test to ensure dolt_ignore exists in HEAD before modifying it, so it correctly appears as "modified" instead of "added" in diff results. The test now inserts a dummy row into dolt_ignore before the initial commit, then deletes it and adds the real pattern after. This makes dolt_ignore show diff_type "modified" rather than "added". Refs: dolthub#5861
codeaucafe
changed the title
codeaucafe
changed the title
Contributor
|
The tests that are failing require using an internal PR. running now. |
macneale4
reviewed
Nov 3, 2025
| if delta.IsAdd() { | ||
| ignoreResult, err := ignorePatterns.IsTableNameIgnored(delta.ToName) | ||
| if err != nil { | ||
| return false // On error, don't ignore |
Contributor
There was a problem hiding this comment.
Ignoring the error isn't ideal. This would effectively mean that the regex was garbage to begin with, and the user should probably know that. Both places this method is used can bubble up errors, so I'll add a commit which does that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Makes
dolt_diff_summarytable function respectdolt_ignorepatterns to matchdolt diffcommand behavior.Changes
dolt_diff_summarydolt_ignorepatterns are now filtered outdolt_*prefixed tablesTesting
Questions/Misc
@macneale4 I think I got this change good for #5861, finally (sorry took me awhile, went out of country and was busy with work to jump back on it immediately). two things to call out for clarity:
Closes: #5861