From a619fd1a2390cb21e348ce989f54e7dad661fa53 Mon Sep 17 00:00:00 2001 From: David Dansby <39511285+codeaucafe@users.noreply.github.com> Date: Wed, 5 Nov 2025 02:01:56 -0800 Subject: [PATCH 01/22] test(diff): add comprehensive unit tests for filter functionality Add Go unit tests for the diff filter feature to provide fast feedback and granular validation of filter logic. Test coverage includes: - diffTypeFilter struct validation (isValid method) - Filter inclusion methods (adds, drops, modifications) - Edge cases (empty strings, typos, case sensitivity) - Consistency checks across all filter types - Constant validation (values, uniqueness, lowercase) - Invalid filter behavior verification Tests added: - 12 test functions - 48+ individual test cases - 100% coverage of diffTypeFilter struct methods These tests complement the existing BATS integration tests and provide unit-level regression protection. Refs: #1430 --- go/cmd/dolt/cli/arg_parser_helpers.go | 1 + go/cmd/dolt/commands/diff.go | 112 ++++++- go/cmd/dolt/commands/diff_filter_test.go | 354 +++++++++++++++++++++++ integration-tests/bats/diff.bats | 220 +++++++++++--- 4 files changed, 646 insertions(+), 41 deletions(-) create mode 100644 go/cmd/dolt/commands/diff_filter_test.go diff --git a/go/cmd/dolt/cli/arg_parser_helpers.go b/go/cmd/dolt/cli/arg_parser_helpers.go index f86badd67c..17cd5f6f19 100644 --- a/go/cmd/dolt/cli/arg_parser_helpers.go +++ b/go/cmd/dolt/cli/arg_parser_helpers.go @@ -320,6 +320,7 @@ func CreateDiffArgParser(isTableFunction bool) *argparser.ArgParser { ap.SupportsString(FormatFlag, "r", "result output format", "How to format diff output. Valid values are tabular, sql, json. Defaults to tabular.") ap.SupportsString(WhereParam, "", "column", "filters columns based on values in the diff. See {{.EmphasisLeft}}dolt diff --help{{.EmphasisRight}} for details.") ap.SupportsInt(LimitParam, "", "record_count", "limits to the first N diffs.") + ap.SupportsString("filter", "", "diff_type", "filters results based on the type of modification (added, modified, removed).") ap.SupportsFlag(StagedFlag, "", "Show only the staged data changes.") ap.SupportsFlag(CachedFlag, "c", "Synonym for --staged") ap.SupportsFlag(MergeBase, "", "Uses merge base of the first commit and second commit (or HEAD if not supplied) as the first commit") diff --git a/go/cmd/dolt/commands/diff.go b/go/cmd/dolt/commands/diff.go index d35a2756b4..4dca44bd75 100644 --- a/go/cmd/dolt/commands/diff.go +++ b/go/cmd/dolt/commands/diff.go @@ -30,6 +30,8 @@ import ( "github.com/gocraft/dbr/v2" "github.com/gocraft/dbr/v2/dialect" + eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1" + "github.com/dolthub/dolt/go/cmd/dolt/cli" "github.com/dolthub/dolt/go/cmd/dolt/errhand" "github.com/dolthub/dolt/go/libraries/doltcore/diff" @@ -42,7 +44,6 @@ import ( "github.com/dolthub/dolt/go/libraries/utils/argparser" "github.com/dolthub/dolt/go/libraries/utils/iohelp" "github.com/dolthub/dolt/go/libraries/utils/set" - eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1" ) type diffOutput int @@ -60,6 +61,11 @@ const ( TabularDiffOutput diffOutput = 1 SQLDiffOutput diffOutput = 2 JsonDiffOutput diffOutput = 3 + + FilterAdds = "added" + FilterModified = "modified" + FilterRemoved = "removed" + NoFilter = "all" ) var diffDocs = cli.CommandDocumentationContent{ @@ -86,6 +92,8 @@ The diffs displayed can be limited to show the first N by providing the paramete To filter which data rows are displayed, use {{.EmphasisLeft}}--where {{.EmphasisRight}}. Table column names in the filter expression must be prefixed with {{.EmphasisLeft}}from_{{.EmphasisRight}} or {{.EmphasisLeft}}to_{{.EmphasisRight}}, e.g. {{.EmphasisLeft}}to_COLUMN_NAME > 100{{.EmphasisRight}} or {{.EmphasisLeft}}from_COLUMN_NAME + to_COLUMN_NAME = 0{{.EmphasisRight}}. +To filter diff output by change type, use {{.EmphasisLeft}}--filter {{.EmphasisRight}} where {{.EmphasisLeft}}{{.EmphasisRight}} is one of {{.EmphasisLeft}}added{{.EmphasisRight}}, {{.EmphasisLeft}}modified{{.EmphasisRight}}, or {{.EmphasisLeft}}removed{{.EmphasisRight}}. The {{.EmphasisLeft}}added{{.EmphasisRight}} filter shows only additions (new tables or rows), {{.EmphasisLeft}}modified{{.EmphasisRight}} shows only modifications (schema changes, renames, or row updates), and {{.EmphasisLeft}}removed{{.EmphasisRight}} shows only deletions (dropped tables or deleted rows). For example, {{.EmphasisLeft}}dolt diff --filter=removed{{.EmphasisRight}} shows only deleted rows and dropped tables. + The {{.EmphasisLeft}}--diff-mode{{.EmphasisRight}} argument controls how modified rows are presented when the format output is set to {{.EmphasisLeft}}tabular{{.EmphasisRight}}. When set to {{.EmphasisLeft}}row{{.EmphasisRight}}, modified rows are presented as old and new rows. When set to {{.EmphasisLeft}}line{{.EmphasisRight}}, modified rows are presented as a single row, and changes are presented using "+" and "-" within the column. When set to {{.EmphasisLeft}}in-place{{.EmphasisRight}}, modified rows are presented as a single row, and changes are presented side-by-side with a color distinction (requires a color-enabled terminal). When set to {{.EmphasisLeft}}context{{.EmphasisRight}}, rows that contain at least one column that spans multiple lines uses {{.EmphasisLeft}}line{{.EmphasisRight}}, while all other rows use {{.EmphasisLeft}}row{{.EmphasisRight}}. The default value is {{.EmphasisLeft}}context{{.EmphasisRight}}. `, Synopsis: []string{ @@ -102,6 +110,7 @@ type diffDisplaySettings struct { where string skinny bool includeCols []string + filter *diffTypeFilter } type diffDatasets struct { @@ -130,6 +139,27 @@ type diffStatistics struct { NewCellCount uint64 } +type diffTypeFilter struct { + filterBy string +} + +func (df *diffTypeFilter) isValid() bool { + return df.filterBy == NoFilter || df.filterBy == FilterAdds || + df.filterBy == FilterModified || df.filterBy == FilterRemoved +} + +func (df *diffTypeFilter) includeAddsOrAll() bool { + return df.filterBy == NoFilter || df.filterBy == FilterAdds +} + +func (df *diffTypeFilter) includeDropsOrAll() bool { + return df.filterBy == NoFilter || df.filterBy == FilterRemoved +} + +func (df *diffTypeFilter) includeModificationsOrAll() bool { + return df.filterBy == NoFilter || df.filterBy == FilterModified +} + type DiffCmd struct{} // Name is returns the name of the Dolt cli command. This is what is used on the command line to invoke the command @@ -220,6 +250,11 @@ func (cmd DiffCmd) validateArgs(apr *argparser.ArgParseResults) errhand.VerboseE return errhand.BuildDError("invalid output format: %s", f).Build() } + filterValue, _ := apr.GetValue("filter") + if filterValue != "" && filterValue != FilterAdds && filterValue != FilterModified && filterValue != FilterRemoved && filterValue != NoFilter { + return errhand.BuildDError("invalid filter: %s. Valid values are: added, modified, removed", filterValue).Build() + } + return nil } @@ -268,6 +303,9 @@ func parseDiffDisplaySettings(apr *argparser.ArgParseResults) *diffDisplaySettin displaySettings.limit, _ = apr.GetInt(cli.LimitParam) displaySettings.where = apr.GetValueOrDefault(cli.WhereParam, "") + filterValue := apr.GetValueOrDefault("filter", "all") + displaySettings.filter = &diffTypeFilter{filterBy: filterValue} + return displaySettings } @@ -646,7 +684,7 @@ func getDeltasBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Context, fromRef, t } func getSchemaDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Context, fromRef, toRef string) ([]diff.TableDeltaSummary, error) { - q, err := dbr.InterpolateForDialect("select * from dolt_schema_diff(?, ?)", []interface{}{fromRef, toRef}, dialect.MySQL) + q, err := dbr.InterpolateForDialect("SELECT * FROM dolt_schema_diff(?, ?)", []interface{}{fromRef, toRef}, dialect.MySQL) if err != nil { return nil, fmt.Errorf("error: unable to interpolate query: %w", err) } @@ -683,7 +721,7 @@ func getSchemaDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Contex } q, err := dbr.InterpolateForDialect( - "select statement_order, statement from dolt_patch(?, ?) where diff_type='schema' and (table_name=? or table_name=?) order by statement_order asc", + "SELECT statement_order, statement FROM dolt_patch(?, ?) WHERE diff_type='schema' AND (table_name=? OR table_name=?) ORDER BY statement_order ASC", []interface{}{fromRef, toRef, fromTable, toTable}, dialect.MySQL) if err != nil { @@ -715,7 +753,7 @@ func getSchemaDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Contex } func getDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Context, fromRef, toRef string) ([]diff.TableDeltaSummary, error) { - q, err := dbr.InterpolateForDialect("select * from dolt_diff_summary(?, ?)", []interface{}{fromRef, toRef}, dialect.MySQL) + q, err := dbr.InterpolateForDialect("SELECT * FROM dolt_diff_summary(?, ?)", []interface{}{fromRef, toRef}, dialect.MySQL) dataDiffRows, err := cli.GetRowsForSql(queryist, sqlCtx, q) if err != nil { return nil, fmt.Errorf("error: unable to get diff summary from %s to %s: %w", fromRef, toRef, err) @@ -816,6 +854,37 @@ func diffUserTables(queryist cli.Queryist, sqlCtx *sql.Context, dArgs *diffArgs) continue } + // Apply table-level filtering based on diff type + if dArgs.filter != nil && dArgs.filter.filterBy != NoFilter { + shouldIncludeTable := false + + // Check if table was added + if delta.IsAdd() && dArgs.filter.includeAddsOrAll() { + shouldIncludeTable = true + } + + // Check if table was dropped + if delta.IsDrop() && dArgs.filter.includeDropsOrAll() { + shouldIncludeTable = true + } + + // Check if table was modified (schema change or rename) + if !delta.IsAdd() && !delta.IsDrop() { + isModified := delta.SchemaChange || delta.IsRename() + if isModified && dArgs.filter.includeModificationsOrAll() { + shouldIncludeTable = true + } + // If no schema/rename changes but has data changes, let it through for row-level filtering + if !isModified && delta.DataChange { + shouldIncludeTable = true + } + } + + if !shouldIncludeTable { + continue // Skip this table but continue processing others + } + } + if strings.HasPrefix(delta.ToTableName.Name, diff.DBPrefix) { verr := diffDatabase(queryist, sqlCtx, delta, dArgs, dw) if verr != nil { @@ -1018,7 +1087,7 @@ func schemaFromCreateTableStmt(createTableStmt string) (schema.Schema, error) { } func getTableDiffStats(queryist cli.Queryist, sqlCtx *sql.Context, tableName, fromRef, toRef string) ([]diffStatistics, error) { - q, err := dbr.InterpolateForDialect("select * from dolt_diff_stat(?, ?, ?)", []interface{}{fromRef, toRef, tableName}, dialect.MySQL) + q, err := dbr.InterpolateForDialect("SELECT * FROM dolt_diff_stat(?, ?, ?)", []interface{}{fromRef, toRef, tableName}, dialect.MySQL) if err != nil { return nil, fmt.Errorf("error interpolating query: %w", err) } @@ -1495,7 +1564,7 @@ func diffRows( } columnNames, format := getColumnNames(fromTableInfo, toTableInfo) - query := fmt.Sprintf("select %s ? from dolt_diff(?, ?, ?)", format) + query := fmt.Sprintf("SELECT %s ? FROM dolt_diff(?, ?, ?)", format) var params []interface{} for _, col := range columnNames { params = append(params, dbr.I(col)) @@ -1708,6 +1777,35 @@ func writeDiffResults( return err } + // Apply row-level filtering based on diff type + if dArgs.filter != nil && dArgs.filter.filterBy != NoFilter { + shouldSkip := false + + // Check oldRow diff type + if oldRow.RowDiff == diff.Added && !dArgs.filter.includeAddsOrAll() { + shouldSkip = true + } else if oldRow.RowDiff == diff.Removed && !dArgs.filter.includeDropsOrAll() { + shouldSkip = true + } else if (oldRow.RowDiff == diff.ModifiedOld || oldRow.RowDiff == diff.ModifiedNew) && + !dArgs.filter.includeModificationsOrAll() { + shouldSkip = true + } + + // Check newRow diff type (it might have a different type) + if newRow.RowDiff == diff.Added && !dArgs.filter.includeAddsOrAll() { + shouldSkip = true + } else if newRow.RowDiff == diff.Removed && !dArgs.filter.includeDropsOrAll() { + shouldSkip = true + } else if (newRow.RowDiff == diff.ModifiedOld || newRow.RowDiff == diff.ModifiedNew) && + !dArgs.filter.includeModificationsOrAll() { + shouldSkip = true + } + + if shouldSkip { + continue // Skip this row and move to the next + } + } + if dArgs.skinny { var filteredOldRow, filteredNewRow diff.RowDiff for i, changeType := range newRow.ColDiffs { @@ -1799,7 +1897,7 @@ func getModifiedCols( func validateWhereClause(queryist cli.Queryist, sqlCtx *sql.Context, dArgs *diffArgs) errhand.VerboseError { // Build a minimal validation query that doesn't depend on having actual tables // We use a subquery approach so the aliased columns are available in the WHERE clause - query := "select * from (select 1 as diff_type, 1 as from_pk, 1 as to_pk) as diff_validation where " + dArgs.where + " limit 0" + query := "SELECT * FROM (SELECT 1 AS diff_type, 1 AS from_pk, 1 AS to_pk) AS diff_validation WHERE " + dArgs.where + " limit 0" _, rowIter, _, err := queryist.Query(sqlCtx, query) if err != nil { return errhand.BuildDError("Error running diff query").AddCause(err).Build() diff --git a/go/cmd/dolt/commands/diff_filter_test.go b/go/cmd/dolt/commands/diff_filter_test.go new file mode 100644 index 0000000000..fee4f6509e --- /dev/null +++ b/go/cmd/dolt/commands/diff_filter_test.go @@ -0,0 +1,354 @@ +// Copyright 2025 Dolthub, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "strings" + "testing" +) + +// ============================================================================ +// diffTypeFilter Struct Tests +// ============================================================================ + +func TestDiffTypeFilter_IsValid(t *testing.T) { + tests := []struct { + name string + filterBy string + want bool + }{ + {"valid: added", FilterAdds, true}, + {"valid: modified", FilterModified, true}, + {"valid: removed", FilterRemoved, true}, + {"valid: all", NoFilter, true}, + {"invalid: empty string", "", false}, + {"invalid: random string", "invalid", false}, + {"invalid: uppercase", "ADDED", false}, + {"invalid: typo addedd", "addedd", false}, + {"invalid: plural adds", "adds", false}, + {"invalid: typo modifiedd", "modifiedd", false}, + {"invalid: typo removedd", "removedd", false}, + {"invalid: insert instead of added", "insert", false}, + {"invalid: update instead of modified", "update", false}, + {"invalid: delete instead of removed", "delete", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + df := &diffTypeFilter{filterBy: tt.filterBy} + got := df.isValid() + if got != tt.want { + t.Errorf("isValid() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestDiffTypeFilter_IncludeAddsOrAll(t *testing.T) { + tests := []struct { + name string + filterBy string + want bool + }{ + {"filter=added returns true", FilterAdds, true}, + {"filter=all returns true", NoFilter, true}, + {"filter=modified returns false", FilterModified, false}, + {"filter=removed returns false", FilterRemoved, false}, + {"empty filter returns false", "", false}, + {"invalid filter returns false", "invalid", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + df := &diffTypeFilter{filterBy: tt.filterBy} + got := df.includeAddsOrAll() + if got != tt.want { + t.Errorf("includeAddsOrAll() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestDiffTypeFilter_IncludeDropsOrAll(t *testing.T) { + tests := []struct { + name string + filterBy string + want bool + }{ + {"filter=removed returns true", FilterRemoved, true}, + {"filter=all returns true", NoFilter, true}, + {"filter=added returns false", FilterAdds, false}, + {"filter=modified returns false", FilterModified, false}, + {"empty filter returns false", "", false}, + {"invalid filter returns false", "invalid", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + df := &diffTypeFilter{filterBy: tt.filterBy} + got := df.includeDropsOrAll() + if got != tt.want { + t.Errorf("includeDropsOrAll() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestDiffTypeFilter_IncludeModificationsOrAll(t *testing.T) { + tests := []struct { + name string + filterBy string + want bool + }{ + {"filter=modified returns true", FilterModified, true}, + {"filter=all returns true", NoFilter, true}, + {"filter=added returns false", FilterAdds, false}, + {"filter=removed returns false", FilterRemoved, false}, + {"empty filter returns false", "", false}, + {"invalid filter returns false", "invalid", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + df := &diffTypeFilter{filterBy: tt.filterBy} + got := df.includeModificationsOrAll() + if got != tt.want { + t.Errorf("includeModificationsOrAll() = %v, want %v", got, tt.want) + } + }) + } +} + +// ============================================================================ +// Edge Case Tests +// ============================================================================ + +func TestDiffTypeFilter_ConsistencyAcrossMethods(t *testing.T) { + // Test that NoFilter returns true for all include methods + t.Run("NoFilter returns true for all methods", func(t *testing.T) { + df := &diffTypeFilter{filterBy: NoFilter} + + if !df.includeAddsOrAll() { + t.Error("NoFilter should include adds") + } + if !df.includeDropsOrAll() { + t.Error("NoFilter should include drops") + } + if !df.includeModificationsOrAll() { + t.Error("NoFilter should include modifications") + } + }) + + // Test that each specific filter only returns true for its method + t.Run("FilterAdds only true for adds", func(t *testing.T) { + df := &diffTypeFilter{filterBy: FilterAdds} + + if !df.includeAddsOrAll() { + t.Error("FilterAdds should include adds") + } + if df.includeDropsOrAll() { + t.Error("FilterAdds should not include drops") + } + if df.includeModificationsOrAll() { + t.Error("FilterAdds should not include modifications") + } + }) + + t.Run("FilterRemoved only true for drops", func(t *testing.T) { + df := &diffTypeFilter{filterBy: FilterRemoved} + + if df.includeAddsOrAll() { + t.Error("FilterRemoved should not include adds") + } + if !df.includeDropsOrAll() { + t.Error("FilterRemoved should include drops") + } + if df.includeModificationsOrAll() { + t.Error("FilterRemoved should not include modifications") + } + }) + + t.Run("FilterModified only true for modifications", func(t *testing.T) { + df := &diffTypeFilter{filterBy: FilterModified} + + if df.includeAddsOrAll() { + t.Error("FilterModified should not include adds") + } + if df.includeDropsOrAll() { + t.Error("FilterModified should not include drops") + } + if !df.includeModificationsOrAll() { + t.Error("FilterModified should include modifications") + } + }) +} + +func TestDiffTypeFilter_InvalidFilterBehavior(t *testing.T) { + // Test that invalid filters consistently return false for all include methods + invalidFilters := []string{"", "invalid", "ADDED", "addedd", "delete"} + + for _, filterValue := range invalidFilters { + t.Run("invalid filter: "+filterValue, func(t *testing.T) { + df := &diffTypeFilter{filterBy: filterValue} + + if !df.isValid() { + // Only test include methods if filter is invalid + if df.includeAddsOrAll() { + t.Error("Invalid filter should not include adds") + } + if df.includeDropsOrAll() { + t.Error("Invalid filter should not include drops") + } + if df.includeModificationsOrAll() { + t.Error("Invalid filter should not include modifications") + } + } + }) + } +} + +// ============================================================================ +// Validation Tests +// ============================================================================ + +func TestDiffCmd_ValidateArgs_ValidFilterValues(t *testing.T) { + tests := []struct { + name string + filterArg string + }{ + {"valid: added", "added"}, + {"valid: modified", "modified"}, + {"valid: removed", "removed"}, + {"valid: all", "all"}, + {"valid: empty (not provided)", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // We're testing that these values pass validation + // The actual validation happens in validateArgs which checks constants + if tt.filterArg != "" { + validValues := []string{FilterAdds, FilterModified, FilterRemoved, NoFilter} + found := false + for _, valid := range validValues { + if tt.filterArg == valid { + found = true + break + } + } + if !found && tt.filterArg != "" { + t.Errorf("Expected %s to be a valid filter value", tt.filterArg) + } + } + }) + } +} + +func TestDiffCmd_ValidateArgs_InvalidFilterValues(t *testing.T) { + tests := []struct { + name string + filterArg string + }{ + {"invalid: random string", "invalid"}, + {"invalid: typo addedd", "addedd"}, + {"invalid: uppercase ADDED", "ADDED"}, + {"invalid: insert", "insert"}, + {"invalid: update", "update"}, + {"invalid: delete", "delete"}, + {"invalid: adds (plural)", "adds"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // These should NOT be in the list of valid values + validValues := []string{FilterAdds, FilterModified, FilterRemoved, NoFilter} + for _, valid := range validValues { + if tt.filterArg == valid { + t.Errorf("Expected %s to be invalid, but it matched %s", tt.filterArg, valid) + } + } + }) + } +} + +// ============================================================================ +// Constant Value Tests +// ============================================================================ + +func TestFilterConstants(t *testing.T) { + // Test that filter constants have expected values + tests := []struct { + name string + constant string + expected string + }{ + {"FilterAdds value", FilterAdds, "added"}, + {"FilterModified value", FilterModified, "modified"}, + {"FilterRemoved value", FilterRemoved, "removed"}, + {"NoFilter value", NoFilter, "all"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.constant != tt.expected { + t.Errorf("Expected %s = %s, got %s", tt.name, tt.expected, tt.constant) + } + }) + } +} + +func TestFilterConstants_AreUnique(t *testing.T) { + // Test that all filter constants are unique + constants := []string{FilterAdds, FilterModified, FilterRemoved, NoFilter} + seen := make(map[string]bool) + + for _, c := range constants { + if seen[c] { + t.Errorf("Duplicate filter constant value: %s", c) + } + seen[c] = true + } + + if len(seen) != 4 { + t.Errorf("Expected 4 unique filter constants, got %d", len(seen)) + } +} + +func TestFilterConstants_AreLowercase(t *testing.T) { + // Test that filter constants are lowercase (convention) + constants := []string{FilterAdds, FilterModified, FilterRemoved, NoFilter} + + for _, c := range constants { + if c != strings.ToLower(c) { + t.Errorf("Filter constant %s should be lowercase", c) + } + } +} + +// ============================================================================ +// Documentation Tests +// ============================================================================ + +func TestDiffTypeFilter_MethodNaming(t *testing.T) { + // Ensure method names are consistent and descriptive + df := &diffTypeFilter{filterBy: NoFilter} + + // These tests just verify the methods exist and are callable + // (compilation will fail if methods are renamed) + _ = df.isValid() + _ = df.includeAddsOrAll() + _ = df.includeDropsOrAll() + _ = df.includeModificationsOrAll() +} diff --git a/integration-tests/bats/diff.bats b/integration-tests/bats/diff.bats index a36eccf6c1..26fa53b17f 100644 --- a/integration-tests/bats/diff.bats +++ b/integration-tests/bats/diff.bats @@ -499,7 +499,7 @@ insert into test values (1,2,3); insert into test values (4,5,6); SQL dolt add . - dolt commit -am "First commit" + dolt add . && dolt commit -m "First commit" dolt sql < Date: Mon, 10 Nov 2025 16:34:49 -0800 Subject: [PATCH 02/22] refactor(diff): prevent empty table hdrs when filtering data-only diffs Implement lazy table header initialization to fix a bug where empty table headers were printed when all rows were filtered out during data-only diffs. This occurred because BeginTable() was called before row filtering, causing headers to print even when no matching rows existed. The solution introduces a lazyRowWriter that delays the BeginTable() call until the first row is actually written. This wrapper is only used when: - A filter is active (added/modified/removed) - The diff is data-only (no schema changes or table renames) Implementation changes: - Add shouldUseLazyHeader() helper to determine when to use lazy initialization based on filter presence and diff type - Add lazyRowWriter type that wraps SqlRowDiffWriter and delays BeginTable() until first WriteRow() or WriteCombinedRow() call - Modify diffUserTable() to skip BeginTable when using lazy writer - Modify diffRows() to conditionally create lazyRowWriter vs normal rowWriter based on shouldUseLazyHeader() check - Add comprehensive unit tests for shouldUseLazyHeader logic and lazyRowWriter behavior (5 test functions, 8+ test cases) - Add mock implementations of diffWriter and SqlRowDiffWriter interfaces to enable testing without database dependencies - Fix BATS test assertions to match actual SQL output format (lowercase type names, MODIFY COLUMN vs DROP/ADD pattern) Test coverage: - TestShouldUseLazyHeader: validates lazy header logic conditions - TestLazyRowWriter_NoRowsWritten: verifies BeginTable not called when no rows written (core lazy behavior) - TestLazyRowWriter_RowsWritten: verifies BeginTable called on first write - TestLazyRowWriter_CombinedRowsWritten: tests combined row writes - TestLazyRowWriter_InitializedOnlyOnce: ensures BeginTable called exactly once Refs: #1430 --- go/cmd/dolt/commands/diff.go | 137 +++++++++-- go/cmd/dolt/commands/diff_filter_test.go | 277 +++++++++++++++++++++-- integration-tests/bats/diff.bats | 77 ++++--- 3 files changed, 419 insertions(+), 72 deletions(-) diff --git a/go/cmd/dolt/commands/diff.go b/go/cmd/dolt/commands/diff.go index 4dca44bd75..2da7011862 100644 --- a/go/cmd/dolt/commands/diff.go +++ b/go/cmd/dolt/commands/diff.go @@ -30,8 +30,6 @@ import ( "github.com/gocraft/dbr/v2" "github.com/gocraft/dbr/v2/dialect" - eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1" - "github.com/dolthub/dolt/go/cmd/dolt/cli" "github.com/dolthub/dolt/go/cmd/dolt/errhand" "github.com/dolthub/dolt/go/libraries/doltcore/diff" @@ -44,6 +42,7 @@ import ( "github.com/dolthub/dolt/go/libraries/utils/argparser" "github.com/dolthub/dolt/go/libraries/utils/iohelp" "github.com/dolthub/dolt/go/libraries/utils/set" + eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1" ) type diffOutput int @@ -160,6 +159,102 @@ func (df *diffTypeFilter) includeModificationsOrAll() bool { return df.filterBy == NoFilter || df.filterBy == FilterModified } +// shouldUseLazyHeader determines if we should delay printing the table header +// until we know there are rows to display. This prevents empty headers when +// all rows are filtered out in data-only diffs. +func shouldUseLazyHeader(dArgs *diffArgs, tableSummary diff.TableDeltaSummary) bool { + return dArgs.filter != nil && dArgs.filter.filterBy != NoFilter && + !tableSummary.SchemaChange && !tableSummary.IsRename() +} + +// lazyRowWriter delays table header output until the first row is written. +// This prevents empty table headers when all rows are filtered out. +type lazyRowWriter struct { + // The real writer, created on first write + realWriter diff.SqlRowDiffWriter + + // Factory to create the real writer + createWriter func() (diff.SqlRowDiffWriter, error) + + // diffWriter to call BeginTable on + dw diffWriter + + // Parameters for BeginTable + fromTableName string + toTableName string + isAdd bool + isDrop bool + + // Has BeginTable been called? + initialized bool +} + +// newLazyRowWriter creates a lazy row writer that delays header output +func newLazyRowWriter( + dw diffWriter, + fromTableName, toTableName string, + isAdd, isDrop bool, + createWriter func() (diff.SqlRowDiffWriter, error), +) *lazyRowWriter { + return &lazyRowWriter{ + createWriter: createWriter, + dw: dw, + fromTableName: fromTableName, + toTableName: toTableName, + isAdd: isAdd, + isDrop: isDrop, + initialized: false, + } +} + +// ensureInitialized calls BeginTable and creates the real writer on first call +func (l *lazyRowWriter) ensureInitialized(ctx *sql.Context) error { + if l.initialized { + return nil + } + + // Call BeginTable to print the header + err := l.dw.BeginTable(l.fromTableName, l.toTableName, l.isAdd, l.isDrop) + if err != nil { + return err + } + + // Create the real writer + realWriter, err := l.createWriter() + if err != nil { + return err + } + + l.realWriter = realWriter + l.initialized = true + return nil +} + +// WriteRow implements diff.SqlRowDiffWriter +func (l *lazyRowWriter) WriteRow(ctx *sql.Context, row sql.Row, diffType diff.ChangeType, colDiffTypes []diff.ChangeType) error { + if err := l.ensureInitialized(ctx); err != nil { + return err + } + return l.realWriter.WriteRow(ctx, row, diffType, colDiffTypes) +} + +// WriteCombinedRow implements diff.SqlRowDiffWriter +func (l *lazyRowWriter) WriteCombinedRow(ctx *sql.Context, oldRow, newRow sql.Row, mode diff.Mode) error { + if err := l.ensureInitialized(ctx); err != nil { + return err + } + return l.realWriter.WriteCombinedRow(ctx, oldRow, newRow, mode) +} + +// Close implements diff.SqlRowDiffWriter +func (l *lazyRowWriter) Close(ctx context.Context) error { + // Only close if we actually created the real writer + if l.initialized && l.realWriter != nil { + return l.realWriter.Close(ctx) + } + return nil +} + type DiffCmd struct{} // Name is returns the name of the Dolt cli command. This is what is used on the command line to invoke the command @@ -684,7 +779,7 @@ func getDeltasBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Context, fromRef, t } func getSchemaDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Context, fromRef, toRef string) ([]diff.TableDeltaSummary, error) { - q, err := dbr.InterpolateForDialect("SELECT * FROM dolt_schema_diff(?, ?)", []interface{}{fromRef, toRef}, dialect.MySQL) + q, err := dbr.InterpolateForDialect("select * from dolt_schema_diff(?, ?)", []interface{}{fromRef, toRef}, dialect.MySQL) if err != nil { return nil, fmt.Errorf("error: unable to interpolate query: %w", err) } @@ -721,7 +816,7 @@ func getSchemaDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Contex } q, err := dbr.InterpolateForDialect( - "SELECT statement_order, statement FROM dolt_patch(?, ?) WHERE diff_type='schema' AND (table_name=? OR table_name=?) ORDER BY statement_order ASC", + "select statement_order, statement from dolt_patch(?, ?) where diff_type='schema' and (table_name=? or table_name=?) order by statement_order asc", []interface{}{fromRef, toRef, fromTable, toTable}, dialect.MySQL) if err != nil { @@ -753,7 +848,7 @@ func getSchemaDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Contex } func getDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Context, fromRef, toRef string) ([]diff.TableDeltaSummary, error) { - q, err := dbr.InterpolateForDialect("SELECT * FROM dolt_diff_summary(?, ?)", []interface{}{fromRef, toRef}, dialect.MySQL) + q, err := dbr.InterpolateForDialect("select * from dolt_diff_summary(?, ?)", []interface{}{fromRef, toRef}, dialect.MySQL) dataDiffRows, err := cli.GetRowsForSql(queryist, sqlCtx, q) if err != nil { return nil, fmt.Errorf("error: unable to get diff summary from %s to %s: %w", fromRef, toRef, err) @@ -1087,7 +1182,7 @@ func schemaFromCreateTableStmt(createTableStmt string) (schema.Schema, error) { } func getTableDiffStats(queryist cli.Queryist, sqlCtx *sql.Context, tableName, fromRef, toRef string) ([]diffStatistics, error) { - q, err := dbr.InterpolateForDialect("SELECT * FROM dolt_diff_stat(?, ?, ?)", []interface{}{fromRef, toRef, tableName}, dialect.MySQL) + q, err := dbr.InterpolateForDialect("select * from dolt_diff_stat(?, ?, ?)", []interface{}{fromRef, toRef, tableName}, dialect.MySQL) if err != nil { return nil, fmt.Errorf("error interpolating query: %w", err) } @@ -1179,7 +1274,7 @@ func diffUserTable( fromTable := tableSummary.FromTableName toTable := tableSummary.ToTableName - if dArgs.diffParts&NameOnlyDiff == 0 { + if dArgs.diffParts&NameOnlyDiff == 0 && !shouldUseLazyHeader(dArgs, tableSummary) { // TODO: schema names err := dw.BeginTable(tableSummary.FromTableName.Name, tableSummary.ToTableName.Name, tableSummary.IsAdd(), tableSummary.IsDrop()) if err != nil { @@ -1514,10 +1609,26 @@ func diffRows( unionSch = unionSchemas(fromSch, toSch) } - // We always instantiate a RowWriter in case the diffWriter needs it to close off any work from schema output - rowWriter, err := dw.RowWriter(fromTableInfo, toTableInfo, tableSummary, unionSch) - if err != nil { - return errhand.VerboseErrorFromError(err) + var rowWriter diff.SqlRowDiffWriter + var err error + if shouldUseLazyHeader(dArgs, tableSummary) { + // Use lazy writer to delay BeginTable until first row write + rowWriter = newLazyRowWriter( + dw, + tableSummary.FromTableName.Name, + tableSummary.ToTableName.Name, + tableSummary.IsAdd(), + tableSummary.IsDrop(), + func() (diff.SqlRowDiffWriter, error) { + return dw.RowWriter(fromTableInfo, toTableInfo, tableSummary, unionSch) + }, + ) + } else { + // We always instantiate a RowWriter in case the diffWriter needs it to close off any work from schema output + rowWriter, err = dw.RowWriter(fromTableInfo, toTableInfo, tableSummary, unionSch) + if err != nil { + return errhand.VerboseErrorFromError(err) + } } // can't diff @@ -1564,7 +1675,7 @@ func diffRows( } columnNames, format := getColumnNames(fromTableInfo, toTableInfo) - query := fmt.Sprintf("SELECT %s ? FROM dolt_diff(?, ?, ?)", format) + query := fmt.Sprintf("select %s ? from dolt_diff(?, ?, ?)", format) var params []interface{} for _, col := range columnNames { params = append(params, dbr.I(col)) @@ -1897,7 +2008,7 @@ func getModifiedCols( func validateWhereClause(queryist cli.Queryist, sqlCtx *sql.Context, dArgs *diffArgs) errhand.VerboseError { // Build a minimal validation query that doesn't depend on having actual tables // We use a subquery approach so the aliased columns are available in the WHERE clause - query := "SELECT * FROM (SELECT 1 AS diff_type, 1 AS from_pk, 1 AS to_pk) AS diff_validation WHERE " + dArgs.where + " limit 0" + query := "select * from (select 1 as diff_type, 1 as from_pk, 1 as to_pk) as diff_validation where " + dArgs.where + " limit 0" _, rowIter, _, err := queryist.Query(sqlCtx, query) if err != nil { return errhand.BuildDError("Error running diff query").AddCause(err).Build() diff --git a/go/cmd/dolt/commands/diff_filter_test.go b/go/cmd/dolt/commands/diff_filter_test.go index fee4f6509e..29607c44b6 100644 --- a/go/cmd/dolt/commands/diff_filter_test.go +++ b/go/cmd/dolt/commands/diff_filter_test.go @@ -15,13 +15,15 @@ package commands import ( + "context" "strings" "testing" -) -// ============================================================================ -// diffTypeFilter Struct Tests -// ============================================================================ + "github.com/dolthub/go-mysql-server/sql" + + "github.com/dolthub/dolt/go/libraries/doltcore/diff" + "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" +) func TestDiffTypeFilter_IsValid(t *testing.T) { tests := []struct { @@ -131,10 +133,6 @@ func TestDiffTypeFilter_IncludeModificationsOrAll(t *testing.T) { } } -// ============================================================================ -// Edge Case Tests -// ============================================================================ - func TestDiffTypeFilter_ConsistencyAcrossMethods(t *testing.T) { // Test that NoFilter returns true for all include methods t.Run("NoFilter returns true for all methods", func(t *testing.T) { @@ -219,10 +217,6 @@ func TestDiffTypeFilter_InvalidFilterBehavior(t *testing.T) { } } -// ============================================================================ -// Validation Tests -// ============================================================================ - func TestDiffCmd_ValidateArgs_ValidFilterValues(t *testing.T) { tests := []struct { name string @@ -283,10 +277,6 @@ func TestDiffCmd_ValidateArgs_InvalidFilterValues(t *testing.T) { } } -// ============================================================================ -// Constant Value Tests -// ============================================================================ - func TestFilterConstants(t *testing.T) { // Test that filter constants have expected values tests := []struct { @@ -337,10 +327,6 @@ func TestFilterConstants_AreLowercase(t *testing.T) { } } -// ============================================================================ -// Documentation Tests -// ============================================================================ - func TestDiffTypeFilter_MethodNaming(t *testing.T) { // Ensure method names are consistent and descriptive df := &diffTypeFilter{filterBy: NoFilter} @@ -352,3 +338,254 @@ func TestDiffTypeFilter_MethodNaming(t *testing.T) { _ = df.includeDropsOrAll() _ = df.includeModificationsOrAll() } + +func TestShouldUseLazyHeader(t *testing.T) { + tests := []struct { + name string + filter *diffTypeFilter + schemaChange bool + isRename bool + expectedResult bool + }{ + { + name: "use lazy: filter active, data-only change", + filter: &diffTypeFilter{filterBy: FilterAdds}, + schemaChange: false, + isRename: false, + expectedResult: true, + }, + { + name: "don't use lazy: no filter", + filter: nil, + schemaChange: false, + isRename: false, + expectedResult: false, + }, + { + name: "don't use lazy: filter is NoFilter", + filter: &diffTypeFilter{filterBy: NoFilter}, + schemaChange: false, + isRename: false, + expectedResult: false, + }, + { + name: "don't use lazy: schema changed", + filter: &diffTypeFilter{filterBy: FilterModified}, + schemaChange: true, + isRename: false, + expectedResult: false, + }, + { + name: "don't use lazy: table renamed", + filter: &diffTypeFilter{filterBy: FilterRemoved}, + schemaChange: false, + isRename: true, + expectedResult: false, + }, + { + name: "don't use lazy: schema changed AND renamed", + filter: &diffTypeFilter{filterBy: FilterAdds}, + schemaChange: true, + isRename: true, + expectedResult: false, + }, + { + name: "use lazy: filter=modified, data-only", + filter: &diffTypeFilter{filterBy: FilterModified}, + schemaChange: false, + isRename: false, + expectedResult: true, + }, + { + name: "use lazy: filter=removed, data-only", + filter: &diffTypeFilter{filterBy: FilterRemoved}, + schemaChange: false, + isRename: false, + expectedResult: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dArgs := &diffArgs{ + diffDisplaySettings: &diffDisplaySettings{ + filter: tt.filter, + }, + } + tableSummary := diff.TableDeltaSummary{ + SchemaChange: tt.schemaChange, + } + // Create a mock rename by setting different from/to names + if tt.isRename { + tableSummary.FromTableName = doltdb.TableName{Name: "old_table"} + tableSummary.ToTableName = doltdb.TableName{Name: "new_table"} + } else { + tableSummary.FromTableName = doltdb.TableName{Name: "table"} + tableSummary.ToTableName = doltdb.TableName{Name: "table"} + } + + result := shouldUseLazyHeader(dArgs, tableSummary) + + if result != tt.expectedResult { + t.Errorf("%s: expected %v, got %v", tt.name, tt.expectedResult, result) + } + }) + } +} + +// mockDiffWriter is a test implementation of diffWriter +type mockDiffWriter struct { + beginTableCalled bool + beginTableError error +} + +func (m *mockDiffWriter) BeginTable(_ /* fromTableName */, _ /* toTableName */ string, _ /* isAdd */, _ /* isDrop */ bool) error { + m.beginTableCalled = true + return m.beginTableError +} + +func (m *mockDiffWriter) WriteTableSchemaDiff(_ /* fromTableInfo */, _ /* toTableInfo */ *diff.TableInfo, _ /* tds */ diff.TableDeltaSummary) error { + return nil +} + +func (m *mockDiffWriter) WriteEventDiff(_ /* ctx */ context.Context, _ /* eventName */, _ /* oldDefn */, _ /* newDefn */ string) error { + return nil +} + +func (m *mockDiffWriter) WriteTriggerDiff(_ /* ctx */ context.Context, _ /* triggerName */, _ /* oldDefn */, _ /* newDefn */ string) error { + return nil +} + +func (m *mockDiffWriter) WriteViewDiff(_ /* ctx */ context.Context, _ /* viewName */, _ /* oldDefn */, _ /* newDefn */ string) error { + return nil +} + +func (m *mockDiffWriter) WriteTableDiffStats(_ /* diffStats */ []diffStatistics, _ /* oldColLen */, _ /* newColLen */ int, _ /* areTablesKeyless */ bool) error { + return nil +} + +func (m *mockDiffWriter) RowWriter(_ /* fromTableInfo */, _ /* toTableInfo */ *diff.TableInfo, _ /* tds */ diff.TableDeltaSummary, _ /* unionSch */ sql.Schema) (diff.SqlRowDiffWriter, error) { + return &mockRowWriter{}, nil +} + +func (m *mockDiffWriter) Close(_ /* ctx */ context.Context) error { + return nil +} + +// mockRowWriter is a test implementation of SqlRowDiffWriter +type mockRowWriter struct { + writeCalled bool + closeCalled bool +} + +func (m *mockRowWriter) WriteRow(_ /* ctx */ *sql.Context, _ /* row */ sql.Row, _ /* diffType */ diff.ChangeType, _ /* colDiffTypes */ []diff.ChangeType) error { + m.writeCalled = true + return nil +} + +func (m *mockRowWriter) WriteCombinedRow(_ /* ctx */ *sql.Context, _ /* oldRow */, _ /* newRow */ sql.Row, _ /* mode */ diff.Mode) error { + m.writeCalled = true + return nil +} + +func (m *mockRowWriter) Close(_ /* ctx */ context.Context) error { + m.closeCalled = true + return nil +} + +func TestLazyRowWriter_NoRowsWritten(t *testing.T) { + mockDW := &mockDiffWriter{} + factory := func() (diff.SqlRowDiffWriter, error) { + return mockDW.RowWriter(nil, nil, diff.TableDeltaSummary{}, nil) + } + + lazyWriter := newLazyRowWriter(mockDW, "fromTable", "toTable", false, false, factory) + + // Close without writing any rows + err := lazyWriter.Close(context.Background()) + if err != nil { + t.Fatalf("Close() returned error: %v", err) + } + + // BeginTable should NEVER have been called + if mockDW.beginTableCalled { + t.Error("BeginTable() was called even though no rows were written - should have been lazy!") + } +} + +func TestLazyRowWriter_RowsWritten(t *testing.T) { + mockDW := &mockDiffWriter{} + factory := func() (diff.SqlRowDiffWriter, error) { + return mockDW.RowWriter(nil, nil, diff.TableDeltaSummary{}, nil) + } + + lazyWriter := newLazyRowWriter(mockDW, "fromTable", "toTable", false, false, factory) + + // Write a row + ctx := sql.NewEmptyContext() + err := lazyWriter.WriteRow(ctx, sql.Row{}, diff.Added, []diff.ChangeType{}) + if err != nil { + t.Fatalf("WriteRow() returned error: %v", err) + } + + // BeginTable should have been called on first write + if !mockDW.beginTableCalled { + t.Error("BeginTable() was NOT called after writing a row - should have been initialized!") + } + + // Close + err = lazyWriter.Close(context.Background()) + if err != nil { + t.Fatalf("Close() returned error: %v", err) + } +} + +func TestLazyRowWriter_CombinedRowsWritten(t *testing.T) { + mockDW := &mockDiffWriter{} + factory := func() (diff.SqlRowDiffWriter, error) { + return mockDW.RowWriter(nil, nil, diff.TableDeltaSummary{}, nil) + } + + lazyWriter := newLazyRowWriter(mockDW, "fromTable", "toTable", false, false, factory) + + // Write a combined row + ctx := sql.NewEmptyContext() + err := lazyWriter.WriteCombinedRow(ctx, sql.Row{}, sql.Row{}, diff.ModeRow) + if err != nil { + t.Fatalf("WriteCombinedRow() returned error: %v", err) + } + + // BeginTable should have been called on first write + if !mockDW.beginTableCalled { + t.Error("BeginTable() was NOT called after writing combined row - should have been initialized!") + } +} + +func TestLazyRowWriter_InitializedOnlyOnce(t *testing.T) { + callCount := 0 + mockDW := &mockDiffWriter{} + factory := func() (diff.SqlRowDiffWriter, error) { + return mockDW.RowWriter(nil, nil, diff.TableDeltaSummary{}, nil) + } + + lazyWriter := newLazyRowWriter(mockDW, "fromTable", "toTable", false, false, factory) + + ctx := sql.NewEmptyContext() + + // Write multiple rows + for i := 0; i < 5; i++ { + mockDW.beginTableCalled = false // Reset flag + err := lazyWriter.WriteRow(ctx, sql.Row{}, diff.Added, []diff.ChangeType{}) + if err != nil { + t.Fatalf("WriteRow() %d returned error: %v", i, err) + } + if mockDW.beginTableCalled { + callCount++ + } + } + + // BeginTable should have been called exactly ONCE (on first write only) + if callCount != 1 { + t.Errorf("BeginTable() called %d times, expected exactly 1", callCount) + } +} diff --git a/integration-tests/bats/diff.bats b/integration-tests/bats/diff.bats index 26fa53b17f..d2dad938a2 100644 --- a/integration-tests/bats/diff.bats +++ b/integration-tests/bats/diff.bats @@ -499,7 +499,7 @@ insert into test values (1,2,3); insert into test values (4,5,6); SQL dolt add . - dolt add . && dolt commit -m "First commit" + dolt commit -am "First commit" dolt sql < Date: Mon, 10 Nov 2025 17:07:27 -0800 Subject: [PATCH 03/22] refactor(diff): extract filter check logic into helper function Extract duplicated row filter checking logic into a reusable shouldSkipDiffType helper function. Refs: #1430 --- go/cmd/dolt/commands/diff.go | 39 ++++++++++++++---------------------- 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/go/cmd/dolt/commands/diff.go b/go/cmd/dolt/commands/diff.go index 2da7011862..ff122dfdf9 100644 --- a/go/cmd/dolt/commands/diff.go +++ b/go/cmd/dolt/commands/diff.go @@ -159,6 +159,19 @@ func (df *diffTypeFilter) includeModificationsOrAll() bool { return df.filterBy == NoFilter || df.filterBy == FilterModified } +// shouldSkipDiffType checks if a row with the given diff type should be skipped based on the filter settings +func shouldSkipDiffType(filter *diffTypeFilter, rowDiffType diff.ChangeType) bool { + if rowDiffType == diff.Added && !filter.includeAddsOrAll() { + return true + } else if rowDiffType == diff.Removed && !filter.includeDropsOrAll() { + return true + } else if (rowDiffType == diff.ModifiedOld || rowDiffType == diff.ModifiedNew) && + !filter.includeModificationsOrAll() { + return true + } + return false +} + // shouldUseLazyHeader determines if we should delay printing the table header // until we know there are rows to display. This prevents empty headers when // all rows are filtered out in data-only diffs. @@ -1890,30 +1903,8 @@ func writeDiffResults( // Apply row-level filtering based on diff type if dArgs.filter != nil && dArgs.filter.filterBy != NoFilter { - shouldSkip := false - - // Check oldRow diff type - if oldRow.RowDiff == diff.Added && !dArgs.filter.includeAddsOrAll() { - shouldSkip = true - } else if oldRow.RowDiff == diff.Removed && !dArgs.filter.includeDropsOrAll() { - shouldSkip = true - } else if (oldRow.RowDiff == diff.ModifiedOld || oldRow.RowDiff == diff.ModifiedNew) && - !dArgs.filter.includeModificationsOrAll() { - shouldSkip = true - } - - // Check newRow diff type (it might have a different type) - if newRow.RowDiff == diff.Added && !dArgs.filter.includeAddsOrAll() { - shouldSkip = true - } else if newRow.RowDiff == diff.Removed && !dArgs.filter.includeDropsOrAll() { - shouldSkip = true - } else if (newRow.RowDiff == diff.ModifiedOld || newRow.RowDiff == diff.ModifiedNew) && - !dArgs.filter.includeModificationsOrAll() { - shouldSkip = true - } - - if shouldSkip { - continue // Skip this row and move to the next + if shouldSkipDiffType(dArgs.filter, oldRow.RowDiff) || shouldSkipDiffType(dArgs.filter, newRow.RowDiff) { + continue } } From 0b70c46398e32938763316fa23bca4d7f2b69c70 Mon Sep 17 00:00:00 2001 From: David Dansby <39511285+codeaucafe@users.noreply.github.com> Date: Wed, 26 Nov 2025 18:42:19 -0800 Subject: [PATCH 04/22] refactor(diff): consolidate filter architecture; use map-based approach Constants and Naming: - Add FilterParam constant to flags.go following existing conventions - Move filter type constants (DiffTypeAdded, DiffTypeModified, DiffTypeRemoved, DiffTypeAll) to table_deltas.go for centralization - Update all references throughout codebase to use diff.DiffType* constants for consistency Validation Consolidation: - Consolidate duplicate validation logic into single isValid() method - Introduce newDiffTypeFilter() constructor for proper initialization - Remove redundant validation checks in parseDiffArgs function Map-Based Filter Architecture: - Refactor diffTypeFilter struct to use map[string]bool instead of string field for more extensible filtering - Replace three separate includeXOrAll() methods with single shouldInclude() method that performs map lookup - Update both table-level and row-level filtering to use unified shouldInclude() approach Row-Level Filtering with DiffType: - Add ChangeTypeToDiffType() helper function in table_deltas.go to convert row-level ChangeType enum to table-level DiffType strings - Refactor shouldSkipDiffType() to shouldSkipRow() using the new conversion helper and map-based filtering - Ensure consistent terminology between table and row filtering by using same DiffType string values throughout Refs: #1430 --- go/cmd/dolt/cli/arg_parser_helpers.go | 2 +- go/cmd/dolt/cli/flags.go | 1 + go/cmd/dolt/commands/diff.go | 112 +++++++++++++-------- go/libraries/doltcore/diff/table_deltas.go | 26 +++++ 4 files changed, 98 insertions(+), 43 deletions(-) diff --git a/go/cmd/dolt/cli/arg_parser_helpers.go b/go/cmd/dolt/cli/arg_parser_helpers.go index 17cd5f6f19..1ccfcc6083 100644 --- a/go/cmd/dolt/cli/arg_parser_helpers.go +++ b/go/cmd/dolt/cli/arg_parser_helpers.go @@ -320,7 +320,7 @@ func CreateDiffArgParser(isTableFunction bool) *argparser.ArgParser { ap.SupportsString(FormatFlag, "r", "result output format", "How to format diff output. Valid values are tabular, sql, json. Defaults to tabular.") ap.SupportsString(WhereParam, "", "column", "filters columns based on values in the diff. See {{.EmphasisLeft}}dolt diff --help{{.EmphasisRight}} for details.") ap.SupportsInt(LimitParam, "", "record_count", "limits to the first N diffs.") - ap.SupportsString("filter", "", "diff_type", "filters results based on the type of modification (added, modified, removed).") + ap.SupportsString(FilterParam, "", "diff_type", "filters results based on the type of modification (added, modified, removed).") ap.SupportsFlag(StagedFlag, "", "Show only the staged data changes.") ap.SupportsFlag(CachedFlag, "c", "Synonym for --staged") ap.SupportsFlag(MergeBase, "", "Uses merge base of the first commit and second commit (or HEAD if not supplied) as the first commit") diff --git a/go/cmd/dolt/cli/flags.go b/go/cmd/dolt/cli/flags.go index 691ddbd973..62a2129c5e 100644 --- a/go/cmd/dolt/cli/flags.go +++ b/go/cmd/dolt/cli/flags.go @@ -99,6 +99,7 @@ const ( SummaryFlag = "summary" WhereParam = "where" LimitParam = "limit" + FilterParam = "filter" MergeBase = "merge-base" DiffMode = "diff-mode" ReverseFlag = "reverse" diff --git a/go/cmd/dolt/commands/diff.go b/go/cmd/dolt/commands/diff.go index ff122dfdf9..bdbdb19f56 100644 --- a/go/cmd/dolt/commands/diff.go +++ b/go/cmd/dolt/commands/diff.go @@ -60,11 +60,6 @@ const ( TabularDiffOutput diffOutput = 1 SQLDiffOutput diffOutput = 2 JsonDiffOutput diffOutput = 3 - - FilterAdds = "added" - FilterModified = "modified" - FilterRemoved = "removed" - NoFilter = "all" ) var diffDocs = cli.CommandDocumentationContent{ @@ -138,45 +133,74 @@ type diffStatistics struct { NewCellCount uint64 } +// diffTypeFilter manages which diff types should be included in the output. +// When filters is nil or empty, all types are included. type diffTypeFilter struct { - filterBy string + // Map of diff type -> should include + // If nil or empty, includes all types + filters map[string]bool } -func (df *diffTypeFilter) isValid() bool { - return df.filterBy == NoFilter || df.filterBy == FilterAdds || - df.filterBy == FilterModified || df.filterBy == FilterRemoved +// newDiffTypeFilter creates a filter for the specified diff type. +// Pass diff.DiffTypeAll or empty string to include all types. +func newDiffTypeFilter(filterType string) *diffTypeFilter { + if filterType == "" || filterType == diff.DiffTypeAll { + return &diffTypeFilter{filters: nil} // nil means include all + } + + return &diffTypeFilter{ + filters: map[string]bool{ + filterType: true, + }, + } } -func (df *diffTypeFilter) includeAddsOrAll() bool { - return df.filterBy == NoFilter || df.filterBy == FilterAdds -} - -func (df *diffTypeFilter) includeDropsOrAll() bool { - return df.filterBy == NoFilter || df.filterBy == FilterRemoved -} - -func (df *diffTypeFilter) includeModificationsOrAll() bool { - return df.filterBy == NoFilter || df.filterBy == FilterModified -} - -// shouldSkipDiffType checks if a row with the given diff type should be skipped based on the filter settings -func shouldSkipDiffType(filter *diffTypeFilter, rowDiffType diff.ChangeType) bool { - if rowDiffType == diff.Added && !filter.includeAddsOrAll() { - return true - } else if rowDiffType == diff.Removed && !filter.includeDropsOrAll() { - return true - } else if (rowDiffType == diff.ModifiedOld || rowDiffType == diff.ModifiedNew) && - !filter.includeModificationsOrAll() { +// shouldInclude checks if the given diff type should be included. +// Uses TableDeltaSummary.DiffType field for table-level filtering. +func (df *diffTypeFilter) shouldInclude(diffType string) bool { + // nil or empty filters means include everything + if df.filters == nil || len(df.filters) == 0 { return true } - return false + + return df.filters[diffType] +} + +// isValid validates the filter configuration +func (df *diffTypeFilter) isValid() bool { + if df.filters == nil { + return true + } + + for filterType := range df.filters { + if filterType != diff.DiffTypeAdded && + filterType != diff.DiffTypeModified && + filterType != diff.DiffTypeRemoved { + return false + } + } + return true +} + +// shouldSkipRow checks if a row should be skipped based on the filter settings. +// Uses the DiffType infrastructure for consistency with table-level filtering. +func shouldSkipRow(filter *diffTypeFilter, rowChangeType diff.ChangeType) bool { + if filter == nil { + return false + } + + // Convert row-level ChangeType to table-level DiffType string + diffType := diff.ChangeTypeToDiffType(rowChangeType) + + // Use the map-based shouldInclude method + return !filter.shouldInclude(diffType) } // shouldUseLazyHeader determines if we should delay printing the table header // until we know there are rows to display. This prevents empty headers when // all rows are filtered out in data-only diffs. func shouldUseLazyHeader(dArgs *diffArgs, tableSummary diff.TableDeltaSummary) bool { - return dArgs.filter != nil && dArgs.filter.filterBy != NoFilter && + return dArgs.filter != nil && dArgs.filter.filters != nil && !tableSummary.SchemaChange && !tableSummary.IsRename() } @@ -358,9 +382,13 @@ func (cmd DiffCmd) validateArgs(apr *argparser.ArgParseResults) errhand.VerboseE return errhand.BuildDError("invalid output format: %s", f).Build() } - filterValue, _ := apr.GetValue("filter") - if filterValue != "" && filterValue != FilterAdds && filterValue != FilterModified && filterValue != FilterRemoved && filterValue != NoFilter { - return errhand.BuildDError("invalid filter: %s. Valid values are: added, modified, removed", filterValue).Build() + filterValue, hasFilter := apr.GetValue(cli.FilterParam) + if hasFilter { + filter := newDiffTypeFilter(filterValue) + if !filter.isValid() { + return errhand.BuildDError("invalid filter: %s. Valid values are: %s, %s, %s", + filterValue, diff.DiffTypeAdded, diff.DiffTypeModified, diff.DiffTypeRemoved).Build() + } } return nil @@ -411,8 +439,8 @@ func parseDiffDisplaySettings(apr *argparser.ArgParseResults) *diffDisplaySettin displaySettings.limit, _ = apr.GetInt(cli.LimitParam) displaySettings.where = apr.GetValueOrDefault(cli.WhereParam, "") - filterValue := apr.GetValueOrDefault("filter", "all") - displaySettings.filter = &diffTypeFilter{filterBy: filterValue} + filterValue := apr.GetValueOrDefault(cli.FilterParam, diff.DiffTypeAll) + displaySettings.filter = newDiffTypeFilter(filterValue) return displaySettings } @@ -963,23 +991,23 @@ func diffUserTables(queryist cli.Queryist, sqlCtx *sql.Context, dArgs *diffArgs) } // Apply table-level filtering based on diff type - if dArgs.filter != nil && dArgs.filter.filterBy != NoFilter { + if dArgs.filter != nil && dArgs.filter.filters != nil { shouldIncludeTable := false // Check if table was added - if delta.IsAdd() && dArgs.filter.includeAddsOrAll() { + if delta.IsAdd() && dArgs.filter.shouldInclude(diff.DiffTypeAdded) { shouldIncludeTable = true } // Check if table was dropped - if delta.IsDrop() && dArgs.filter.includeDropsOrAll() { + if delta.IsDrop() && dArgs.filter.shouldInclude(diff.DiffTypeRemoved) { shouldIncludeTable = true } // Check if table was modified (schema change or rename) if !delta.IsAdd() && !delta.IsDrop() { isModified := delta.SchemaChange || delta.IsRename() - if isModified && dArgs.filter.includeModificationsOrAll() { + if isModified && dArgs.filter.shouldInclude(diff.DiffTypeModified) { shouldIncludeTable = true } // If no schema/rename changes but has data changes, let it through for row-level filtering @@ -1902,8 +1930,8 @@ func writeDiffResults( } // Apply row-level filtering based on diff type - if dArgs.filter != nil && dArgs.filter.filterBy != NoFilter { - if shouldSkipDiffType(dArgs.filter, oldRow.RowDiff) || shouldSkipDiffType(dArgs.filter, newRow.RowDiff) { + if dArgs.filter != nil { + if shouldSkipRow(dArgs.filter, oldRow.RowDiff) || shouldSkipRow(dArgs.filter, newRow.RowDiff) { continue } } diff --git a/go/libraries/doltcore/diff/table_deltas.go b/go/libraries/doltcore/diff/table_deltas.go index 201b555011..60728941f2 100644 --- a/go/libraries/doltcore/diff/table_deltas.go +++ b/go/libraries/doltcore/diff/table_deltas.go @@ -39,6 +39,16 @@ const ( RemovedTable ) +// Filter type constants for diff filtering. +// These correspond to the string values used in the --filter flag and +// are stored in TableDeltaSummary.DiffType field. +const ( + DiffTypeAdded = "added" + DiffTypeModified = "modified" + DiffTypeRemoved = "removed" + DiffTypeAll = "all" +) + const DBPrefix = "__DATABASE__" type TableInfo struct { @@ -97,6 +107,22 @@ func (tds TableDeltaSummary) IsRename() bool { return tds.FromTableName != tds.ToTableName } +// ChangeTypeToDiffType converts a row-level ChangeType to a table-level DiffType string. +// This allows row-level filtering to use the same DiffType infrastructure as table-level filtering. +func ChangeTypeToDiffType(ct ChangeType) string { + switch ct { + case Added: + return DiffTypeAdded + case Removed: + return DiffTypeRemoved + case ModifiedOld, ModifiedNew: + // Both ModifiedOld and ModifiedNew represent the same logical change: modified + return DiffTypeModified + default: + return "" + } +} + // GetStagedUnstagedTableDeltas represents staged and unstaged changes as TableDelta slices. func GetStagedUnstagedTableDeltas(ctx context.Context, roots doltdb.Roots) (staged, unstaged []TableDelta, err error) { staged, err = GetTableDeltas(ctx, roots.Head, roots.Staged) From cd81c2d60af3b591f654a04d7032a167856328ad Mon Sep 17 00:00:00 2001 From: David Dansby <39511285+codeaucafe@users.noreply.github.com> Date: Wed, 26 Nov 2025 19:03:10 -0800 Subject: [PATCH 05/22] refactor(diff): simplify lazyRowWriter using callback pattern Refactor lazyRowWriter to use a cleaner callback-based architecture that reduces struct complexity from 8 fields to 2 fields. This addresses PR review feedback requesting simplification of the lazy header implementation. Changes: - Replace parameter storage with closure-based callback pattern that captures BeginTable parameters from outer scope - Eliminate separate ensureInitialized() method in favor of inline initialization check in WriteRow() and WriteCombinedRow() - Remove initialized bool field by using callback presence (nil check) to track initialization state - Always create RowWriter upfront and only delay BeginTable call, eliminating the need for writer factory function - Simplify Close() method to always delegate to wrapped writer Refs: #1430 --- go/cmd/dolt/commands/diff.go | 132 +++++++++++++---------------------- 1 file changed, 48 insertions(+), 84 deletions(-) diff --git a/go/cmd/dolt/commands/diff.go b/go/cmd/dolt/commands/diff.go index bdbdb19f56..c1b28c8476 100644 --- a/go/cmd/dolt/commands/diff.go +++ b/go/cmd/dolt/commands/diff.go @@ -30,6 +30,8 @@ import ( "github.com/gocraft/dbr/v2" "github.com/gocraft/dbr/v2/dialect" + eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1" + "github.com/dolthub/dolt/go/cmd/dolt/cli" "github.com/dolthub/dolt/go/cmd/dolt/errhand" "github.com/dolthub/dolt/go/libraries/doltcore/diff" @@ -42,7 +44,6 @@ import ( "github.com/dolthub/dolt/go/libraries/utils/argparser" "github.com/dolthub/dolt/go/libraries/utils/iohelp" "github.com/dolthub/dolt/go/libraries/utils/set" - eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1" ) type diffOutput int @@ -204,92 +205,55 @@ func shouldUseLazyHeader(dArgs *diffArgs, tableSummary diff.TableDeltaSummary) b !tableSummary.SchemaChange && !tableSummary.IsRename() } -// lazyRowWriter delays table header output until the first row is written. -// This prevents empty table headers when all rows are filtered out. +// lazyRowWriter wraps a SqlRowDiffWriter and delays calling BeginTable +// until the first row is actually written. This prevents empty table headers +// when all rows are filtered out. type lazyRowWriter struct { - // The real writer, created on first write - realWriter diff.SqlRowDiffWriter + writer diff.SqlRowDiffWriter - // Factory to create the real writer - createWriter func() (diff.SqlRowDiffWriter, error) - - // diffWriter to call BeginTable on - dw diffWriter - - // Parameters for BeginTable - fromTableName string - toTableName string - isAdd bool - isDrop bool - - // Has BeginTable been called? - initialized bool + // Callback to invoke before first write + // Set to nil after first call + onFirstWrite func() error } -// newLazyRowWriter creates a lazy row writer that delays header output -func newLazyRowWriter( - dw diffWriter, - fromTableName, toTableName string, - isAdd, isDrop bool, - createWriter func() (diff.SqlRowDiffWriter, error), -) *lazyRowWriter { +// newLazyRowWriter creates a lazy writer that wraps the given writer. +// The onFirstWrite callback is invoked exactly once before the first write. +func newLazyRowWriter(writer diff.SqlRowDiffWriter, onFirstWrite func() error) *lazyRowWriter { return &lazyRowWriter{ - createWriter: createWriter, - dw: dw, - fromTableName: fromTableName, - toTableName: toTableName, - isAdd: isAdd, - isDrop: isDrop, - initialized: false, + writer: writer, + onFirstWrite: onFirstWrite, } } -// ensureInitialized calls BeginTable and creates the real writer on first call -func (l *lazyRowWriter) ensureInitialized(ctx *sql.Context) error { - if l.initialized { - return nil - } - - // Call BeginTable to print the header - err := l.dw.BeginTable(l.fromTableName, l.toTableName, l.isAdd, l.isDrop) - if err != nil { - return err - } - - // Create the real writer - realWriter, err := l.createWriter() - if err != nil { - return err - } - - l.realWriter = realWriter - l.initialized = true - return nil -} - // WriteRow implements diff.SqlRowDiffWriter func (l *lazyRowWriter) WriteRow(ctx *sql.Context, row sql.Row, diffType diff.ChangeType, colDiffTypes []diff.ChangeType) error { - if err := l.ensureInitialized(ctx); err != nil { - return err + // Initialize on first write + if l.onFirstWrite != nil { + if err := l.onFirstWrite(); err != nil { + return err + } + l.onFirstWrite = nil // Prevent double-initialization } - return l.realWriter.WriteRow(ctx, row, diffType, colDiffTypes) + + return l.writer.WriteRow(ctx, row, diffType, colDiffTypes) } // WriteCombinedRow implements diff.SqlRowDiffWriter func (l *lazyRowWriter) WriteCombinedRow(ctx *sql.Context, oldRow, newRow sql.Row, mode diff.Mode) error { - if err := l.ensureInitialized(ctx); err != nil { - return err + // Initialize on first write + if l.onFirstWrite != nil { + if err := l.onFirstWrite(); err != nil { + return err + } + l.onFirstWrite = nil } - return l.realWriter.WriteCombinedRow(ctx, oldRow, newRow, mode) + + return l.writer.WriteCombinedRow(ctx, oldRow, newRow, mode) } // Close implements diff.SqlRowDiffWriter func (l *lazyRowWriter) Close(ctx context.Context) error { - // Only close if we actually created the real writer - if l.initialized && l.realWriter != nil { - return l.realWriter.Close(ctx) - } - return nil + return l.writer.Close(ctx) } type DiffCmd struct{} @@ -1650,26 +1614,26 @@ func diffRows( unionSch = unionSchemas(fromSch, toSch) } + // We always instantiate a RowWriter in case the diffWriter needs it to close off any work from schema output var rowWriter diff.SqlRowDiffWriter - var err error + realWriter, err := dw.RowWriter(fromTableInfo, toTableInfo, tableSummary, unionSch) + if err != nil { + return errhand.VerboseErrorFromError(err) + } + if shouldUseLazyHeader(dArgs, tableSummary) { - // Use lazy writer to delay BeginTable until first row write - rowWriter = newLazyRowWriter( - dw, - tableSummary.FromTableName.Name, - tableSummary.ToTableName.Name, - tableSummary.IsAdd(), - tableSummary.IsDrop(), - func() (diff.SqlRowDiffWriter, error) { - return dw.RowWriter(fromTableInfo, toTableInfo, tableSummary, unionSch) - }, - ) - } else { - // We always instantiate a RowWriter in case the diffWriter needs it to close off any work from schema output - rowWriter, err = dw.RowWriter(fromTableInfo, toTableInfo, tableSummary, unionSch) - if err != nil { - return errhand.VerboseErrorFromError(err) + // Wrap with lazy writer to delay BeginTable until first row write + onFirstWrite := func() error { + return dw.BeginTable( + tableSummary.FromTableName.Name, + tableSummary.ToTableName.Name, + tableSummary.IsAdd(), + tableSummary.IsDrop(), + ) } + rowWriter = newLazyRowWriter(realWriter, onFirstWrite) + } else { + rowWriter = realWriter } // can't diff From 4a3ff0e8834c9803ae956ed563b6c0e10cf105f2 Mon Sep 17 00:00:00 2001 From: David Dansby <39511285+codeaucafe@users.noreply.github.com> Date: Fri, 28 Nov 2025 18:42:29 -0800 Subject: [PATCH 06/22] refactor(diff): address PR review feedback and fix row filtering Address @elianddb feedback on PR #10030 by standardizing DiffType usage, simplifying filter logic, and fixing row-level filtering. Changes: - Fix row filtering bug: add early return for ChangeType.None to prevent incorrectly skipping added/removed rows - Standardize DiffType: replace string literals with constants (DiffTypeAdded/Modified/Removed) in table_deltas.go and merge.go - Simplify table filtering: use delta.DiffType directly - Update tests: rewrite to use map-based architecture and newDiffTypeFilter() constructor Refs: #1430 --- go/cmd/dolt/commands/diff.go | 55 ++- go/cmd/dolt/commands/diff_filter_test.go | 391 +++++++++------------ go/cmd/dolt/commands/merge.go | 6 +- go/libraries/doltcore/diff/table_deltas.go | 8 +- 4 files changed, 196 insertions(+), 264 deletions(-) diff --git a/go/cmd/dolt/commands/diff.go b/go/cmd/dolt/commands/diff.go index c1b28c8476..6365714070 100644 --- a/go/cmd/dolt/commands/diff.go +++ b/go/cmd/dolt/commands/diff.go @@ -190,6 +190,11 @@ func shouldSkipRow(filter *diffTypeFilter, rowChangeType diff.ChangeType) bool { return false } + // Don't filter None - it represents "no row" on one side of the diff + if rowChangeType == diff.None { + return false + } + // Convert row-level ChangeType to table-level DiffType string diffType := diff.ChangeTypeToDiffType(rowChangeType) @@ -808,13 +813,13 @@ func getSchemaDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Contex tableName = fromTable } case fromTable == "": - diffType = "added" + diffType = diff.DiffTypeAdded tableName = toTable case toTable == "": - diffType = "dropped" + diffType = diff.DiffTypeRemoved tableName = fromTable case fromTable != "" && toTable != "" && fromTable != toTable: - diffType = "renamed" + diffType = diff.DiffTypeModified // Renamed tables are treated as modified tableName = toTable default: return nil, fmt.Errorf("error: unexpected schema diff case: fromTable='%s', toTable='%s'", fromTable, toTable) @@ -876,14 +881,17 @@ func getDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Context, fro } switch summary.DiffType { - case "dropped": + case diff.DiffTypeRemoved: summary.TableName = summary.FromTableName - case "added": + case diff.DiffTypeAdded: summary.TableName = summary.ToTableName - case "renamed": - summary.TableName = summary.ToTableName - case "modified": - summary.TableName = summary.FromTableName + case diff.DiffTypeModified: + // For renamed tables, use ToTableName; for other modifications, use FromTableName + if summary.FromTableName.Name != summary.ToTableName.Name { + summary.TableName = summary.ToTableName + } else { + summary.TableName = summary.FromTableName + } default: return nil, fmt.Errorf("error: unexpected diff type '%s'", summary.DiffType) } @@ -956,32 +964,11 @@ func diffUserTables(queryist cli.Queryist, sqlCtx *sql.Context, dArgs *diffArgs) // Apply table-level filtering based on diff type if dArgs.filter != nil && dArgs.filter.filters != nil { - shouldIncludeTable := false + // For data-only changes (no schema/rename), always let them through for row-level filtering + isDataOnlyChange := !delta.SchemaChange && !delta.IsRename() && delta.DataChange - // Check if table was added - if delta.IsAdd() && dArgs.filter.shouldInclude(diff.DiffTypeAdded) { - shouldIncludeTable = true - } - - // Check if table was dropped - if delta.IsDrop() && dArgs.filter.shouldInclude(diff.DiffTypeRemoved) { - shouldIncludeTable = true - } - - // Check if table was modified (schema change or rename) - if !delta.IsAdd() && !delta.IsDrop() { - isModified := delta.SchemaChange || delta.IsRename() - if isModified && dArgs.filter.shouldInclude(diff.DiffTypeModified) { - shouldIncludeTable = true - } - // If no schema/rename changes but has data changes, let it through for row-level filtering - if !isModified && delta.DataChange { - shouldIncludeTable = true - } - } - - if !shouldIncludeTable { - continue // Skip this table but continue processing others + if !isDataOnlyChange && !dArgs.filter.shouldInclude(delta.DiffType) { + continue // Skip this table } } diff --git a/go/cmd/dolt/commands/diff_filter_test.go b/go/cmd/dolt/commands/diff_filter_test.go index 29607c44b6..51109ef8bb 100644 --- a/go/cmd/dolt/commands/diff_filter_test.go +++ b/go/cmd/dolt/commands/diff_filter_test.go @@ -31,11 +31,11 @@ func TestDiffTypeFilter_IsValid(t *testing.T) { filterBy string want bool }{ - {"valid: added", FilterAdds, true}, - {"valid: modified", FilterModified, true}, - {"valid: removed", FilterRemoved, true}, - {"valid: all", NoFilter, true}, - {"invalid: empty string", "", false}, + {"valid: added", diff.DiffTypeAdded, true}, + {"valid: modified", diff.DiffTypeModified, true}, + {"valid: removed", diff.DiffTypeRemoved, true}, + {"valid: all", diff.DiffTypeAll, true}, + {"invalid: empty string with nil filter", "", true}, // nil filter is valid {"invalid: random string", "invalid", false}, {"invalid: uppercase", "ADDED", false}, {"invalid: typo addedd", "addedd", false}, @@ -49,7 +49,7 @@ func TestDiffTypeFilter_IsValid(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - df := &diffTypeFilter{filterBy: tt.filterBy} + df := newDiffTypeFilter(tt.filterBy) got := df.isValid() if got != tt.want { t.Errorf("isValid() = %v, want %v", got, tt.want) @@ -58,220 +58,120 @@ func TestDiffTypeFilter_IsValid(t *testing.T) { } } -func TestDiffTypeFilter_IncludeAddsOrAll(t *testing.T) { +func TestDiffTypeFilter_ShouldInclude(t *testing.T) { tests := []struct { - name string - filterBy string - want bool + name string + filterType string + checkType string + want bool }{ - {"filter=added returns true", FilterAdds, true}, - {"filter=all returns true", NoFilter, true}, - {"filter=modified returns false", FilterModified, false}, - {"filter=removed returns false", FilterRemoved, false}, - {"empty filter returns false", "", false}, - {"invalid filter returns false", "invalid", false}, + // Testing with filter=added + {"filter=added, check added", diff.DiffTypeAdded, diff.DiffTypeAdded, true}, + {"filter=added, check modified", diff.DiffTypeAdded, diff.DiffTypeModified, false}, + {"filter=added, check removed", diff.DiffTypeAdded, diff.DiffTypeRemoved, false}, + + // Testing with filter=modified + {"filter=modified, check added", diff.DiffTypeModified, diff.DiffTypeAdded, false}, + {"filter=modified, check modified", diff.DiffTypeModified, diff.DiffTypeModified, true}, + {"filter=modified, check removed", diff.DiffTypeModified, diff.DiffTypeRemoved, false}, + + // Testing with filter=removed + {"filter=removed, check added", diff.DiffTypeRemoved, diff.DiffTypeAdded, false}, + {"filter=removed, check modified", diff.DiffTypeRemoved, diff.DiffTypeModified, false}, + {"filter=removed, check removed", diff.DiffTypeRemoved, diff.DiffTypeRemoved, true}, + + // Testing with filter=all + {"filter=all, check added", diff.DiffTypeAll, diff.DiffTypeAdded, true}, + {"filter=all, check modified", diff.DiffTypeAll, diff.DiffTypeModified, true}, + {"filter=all, check removed", diff.DiffTypeAll, diff.DiffTypeRemoved, true}, + + // Testing with empty filter (nil filters map) + {"filter=empty, check added", "", diff.DiffTypeAdded, true}, + {"filter=empty, check modified", "", diff.DiffTypeModified, true}, + {"filter=empty, check removed", "", diff.DiffTypeRemoved, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - df := &diffTypeFilter{filterBy: tt.filterBy} - got := df.includeAddsOrAll() + df := newDiffTypeFilter(tt.filterType) + got := df.shouldInclude(tt.checkType) if got != tt.want { - t.Errorf("includeAddsOrAll() = %v, want %v", got, tt.want) - } - }) - } -} - -func TestDiffTypeFilter_IncludeDropsOrAll(t *testing.T) { - tests := []struct { - name string - filterBy string - want bool - }{ - {"filter=removed returns true", FilterRemoved, true}, - {"filter=all returns true", NoFilter, true}, - {"filter=added returns false", FilterAdds, false}, - {"filter=modified returns false", FilterModified, false}, - {"empty filter returns false", "", false}, - {"invalid filter returns false", "invalid", false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - df := &diffTypeFilter{filterBy: tt.filterBy} - got := df.includeDropsOrAll() - if got != tt.want { - t.Errorf("includeDropsOrAll() = %v, want %v", got, tt.want) - } - }) - } -} - -func TestDiffTypeFilter_IncludeModificationsOrAll(t *testing.T) { - tests := []struct { - name string - filterBy string - want bool - }{ - {"filter=modified returns true", FilterModified, true}, - {"filter=all returns true", NoFilter, true}, - {"filter=added returns false", FilterAdds, false}, - {"filter=removed returns false", FilterRemoved, false}, - {"empty filter returns false", "", false}, - {"invalid filter returns false", "invalid", false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - df := &diffTypeFilter{filterBy: tt.filterBy} - got := df.includeModificationsOrAll() - if got != tt.want { - t.Errorf("includeModificationsOrAll() = %v, want %v", got, tt.want) + t.Errorf("shouldInclude(%s) = %v, want %v", tt.checkType, got, tt.want) } }) } } func TestDiffTypeFilter_ConsistencyAcrossMethods(t *testing.T) { - // Test that NoFilter returns true for all include methods - t.Run("NoFilter returns true for all methods", func(t *testing.T) { - df := &diffTypeFilter{filterBy: NoFilter} + // Test that filter=all returns true for all diff types + t.Run("filter=all returns true for all types", func(t *testing.T) { + df := newDiffTypeFilter(diff.DiffTypeAll) - if !df.includeAddsOrAll() { - t.Error("NoFilter should include adds") + if !df.shouldInclude(diff.DiffTypeAdded) { + t.Error("filter=all should include added") } - if !df.includeDropsOrAll() { - t.Error("NoFilter should include drops") + if !df.shouldInclude(diff.DiffTypeRemoved) { + t.Error("filter=all should include removed") } - if !df.includeModificationsOrAll() { - t.Error("NoFilter should include modifications") + if !df.shouldInclude(diff.DiffTypeModified) { + t.Error("filter=all should include modified") } }) - // Test that each specific filter only returns true for its method - t.Run("FilterAdds only true for adds", func(t *testing.T) { - df := &diffTypeFilter{filterBy: FilterAdds} + // Test that each specific filter only returns true for its type + t.Run("filter=added only includes added", func(t *testing.T) { + df := newDiffTypeFilter(diff.DiffTypeAdded) - if !df.includeAddsOrAll() { - t.Error("FilterAdds should include adds") + if !df.shouldInclude(diff.DiffTypeAdded) { + t.Error("filter=added should include added") } - if df.includeDropsOrAll() { - t.Error("FilterAdds should not include drops") + if df.shouldInclude(diff.DiffTypeRemoved) { + t.Error("filter=added should not include removed") } - if df.includeModificationsOrAll() { - t.Error("FilterAdds should not include modifications") + if df.shouldInclude(diff.DiffTypeModified) { + t.Error("filter=added should not include modified") } }) - t.Run("FilterRemoved only true for drops", func(t *testing.T) { - df := &diffTypeFilter{filterBy: FilterRemoved} + t.Run("filter=removed only includes removed", func(t *testing.T) { + df := newDiffTypeFilter(diff.DiffTypeRemoved) - if df.includeAddsOrAll() { - t.Error("FilterRemoved should not include adds") + if df.shouldInclude(diff.DiffTypeAdded) { + t.Error("filter=removed should not include added") } - if !df.includeDropsOrAll() { - t.Error("FilterRemoved should include drops") + if !df.shouldInclude(diff.DiffTypeRemoved) { + t.Error("filter=removed should include removed") } - if df.includeModificationsOrAll() { - t.Error("FilterRemoved should not include modifications") + if df.shouldInclude(diff.DiffTypeModified) { + t.Error("filter=removed should not include modified") } }) - t.Run("FilterModified only true for modifications", func(t *testing.T) { - df := &diffTypeFilter{filterBy: FilterModified} + t.Run("filter=modified only includes modified", func(t *testing.T) { + df := newDiffTypeFilter(diff.DiffTypeModified) - if df.includeAddsOrAll() { - t.Error("FilterModified should not include adds") + if df.shouldInclude(diff.DiffTypeAdded) { + t.Error("filter=modified should not include added") } - if df.includeDropsOrAll() { - t.Error("FilterModified should not include drops") + if df.shouldInclude(diff.DiffTypeRemoved) { + t.Error("filter=modified should not include removed") } - if !df.includeModificationsOrAll() { - t.Error("FilterModified should include modifications") + if !df.shouldInclude(diff.DiffTypeModified) { + t.Error("filter=modified should include modified") } }) } func TestDiffTypeFilter_InvalidFilterBehavior(t *testing.T) { - // Test that invalid filters consistently return false for all include methods - invalidFilters := []string{"", "invalid", "ADDED", "addedd", "delete"} + // Test that invalid filters return false for isValid + invalidFilters := []string{"invalid", "ADDED", "addedd", "delete"} for _, filterValue := range invalidFilters { t.Run("invalid filter: "+filterValue, func(t *testing.T) { - df := &diffTypeFilter{filterBy: filterValue} + df := newDiffTypeFilter(filterValue) - if !df.isValid() { - // Only test include methods if filter is invalid - if df.includeAddsOrAll() { - t.Error("Invalid filter should not include adds") - } - if df.includeDropsOrAll() { - t.Error("Invalid filter should not include drops") - } - if df.includeModificationsOrAll() { - t.Error("Invalid filter should not include modifications") - } - } - }) - } -} - -func TestDiffCmd_ValidateArgs_ValidFilterValues(t *testing.T) { - tests := []struct { - name string - filterArg string - }{ - {"valid: added", "added"}, - {"valid: modified", "modified"}, - {"valid: removed", "removed"}, - {"valid: all", "all"}, - {"valid: empty (not provided)", ""}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // We're testing that these values pass validation - // The actual validation happens in validateArgs which checks constants - if tt.filterArg != "" { - validValues := []string{FilterAdds, FilterModified, FilterRemoved, NoFilter} - found := false - for _, valid := range validValues { - if tt.filterArg == valid { - found = true - break - } - } - if !found && tt.filterArg != "" { - t.Errorf("Expected %s to be a valid filter value", tt.filterArg) - } - } - }) - } -} - -func TestDiffCmd_ValidateArgs_InvalidFilterValues(t *testing.T) { - tests := []struct { - name string - filterArg string - }{ - {"invalid: random string", "invalid"}, - {"invalid: typo addedd", "addedd"}, - {"invalid: uppercase ADDED", "ADDED"}, - {"invalid: insert", "insert"}, - {"invalid: update", "update"}, - {"invalid: delete", "delete"}, - {"invalid: adds (plural)", "adds"}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // These should NOT be in the list of valid values - validValues := []string{FilterAdds, FilterModified, FilterRemoved, NoFilter} - for _, valid := range validValues { - if tt.filterArg == valid { - t.Errorf("Expected %s to be invalid, but it matched %s", tt.filterArg, valid) - } + if df.isValid() { + t.Errorf("Filter %s should be invalid", filterValue) } }) } @@ -284,10 +184,10 @@ func TestFilterConstants(t *testing.T) { constant string expected string }{ - {"FilterAdds value", FilterAdds, "added"}, - {"FilterModified value", FilterModified, "modified"}, - {"FilterRemoved value", FilterRemoved, "removed"}, - {"NoFilter value", NoFilter, "all"}, + {"DiffTypeAdded value", diff.DiffTypeAdded, "added"}, + {"DiffTypeModified value", diff.DiffTypeModified, "modified"}, + {"DiffTypeRemoved value", diff.DiffTypeRemoved, "removed"}, + {"DiffTypeAll value", diff.DiffTypeAll, "all"}, } for _, tt := range tests { @@ -301,7 +201,7 @@ func TestFilterConstants(t *testing.T) { func TestFilterConstants_AreUnique(t *testing.T) { // Test that all filter constants are unique - constants := []string{FilterAdds, FilterModified, FilterRemoved, NoFilter} + constants := []string{diff.DiffTypeAdded, diff.DiffTypeModified, diff.DiffTypeRemoved, diff.DiffTypeAll} seen := make(map[string]bool) for _, c := range constants { @@ -318,7 +218,7 @@ func TestFilterConstants_AreUnique(t *testing.T) { func TestFilterConstants_AreLowercase(t *testing.T) { // Test that filter constants are lowercase (convention) - constants := []string{FilterAdds, FilterModified, FilterRemoved, NoFilter} + constants := []string{diff.DiffTypeAdded, diff.DiffTypeModified, diff.DiffTypeRemoved, diff.DiffTypeAll} for _, c := range constants { if c != strings.ToLower(c) { @@ -327,78 +227,66 @@ func TestFilterConstants_AreLowercase(t *testing.T) { } } -func TestDiffTypeFilter_MethodNaming(t *testing.T) { - // Ensure method names are consistent and descriptive - df := &diffTypeFilter{filterBy: NoFilter} - - // These tests just verify the methods exist and are callable - // (compilation will fail if methods are renamed) - _ = df.isValid() - _ = df.includeAddsOrAll() - _ = df.includeDropsOrAll() - _ = df.includeModificationsOrAll() -} - func TestShouldUseLazyHeader(t *testing.T) { tests := []struct { name string - filter *diffTypeFilter + filterType string schemaChange bool isRename bool expectedResult bool }{ { name: "use lazy: filter active, data-only change", - filter: &diffTypeFilter{filterBy: FilterAdds}, + filterType: diff.DiffTypeAdded, schemaChange: false, isRename: false, expectedResult: true, }, { name: "don't use lazy: no filter", - filter: nil, + filterType: "", schemaChange: false, isRename: false, expectedResult: false, }, { - name: "don't use lazy: filter is NoFilter", - filter: &diffTypeFilter{filterBy: NoFilter}, + name: "don't use lazy: filter is all", + filterType: diff.DiffTypeAll, schemaChange: false, isRename: false, expectedResult: false, }, { name: "don't use lazy: schema changed", - filter: &diffTypeFilter{filterBy: FilterModified}, + filterType: diff.DiffTypeModified, schemaChange: true, isRename: false, expectedResult: false, }, { name: "don't use lazy: table renamed", - filter: &diffTypeFilter{filterBy: FilterRemoved}, + filterType: diff.DiffTypeRemoved, schemaChange: false, isRename: true, expectedResult: false, }, { name: "don't use lazy: schema changed AND renamed", - filter: &diffTypeFilter{filterBy: FilterAdds}, + filterType: diff.DiffTypeAdded, schemaChange: true, isRename: true, expectedResult: false, }, { name: "use lazy: filter=modified, data-only", - filter: &diffTypeFilter{filterBy: FilterModified}, + filterType: diff.DiffTypeModified, schemaChange: false, isRename: false, expectedResult: true, }, { name: "use lazy: filter=removed, data-only", - filter: &diffTypeFilter{filterBy: FilterRemoved}, + filterType: diff.DiffTypeRemoved, schemaChange: false, isRename: false, expectedResult: true, @@ -407,9 +295,14 @@ func TestShouldUseLazyHeader(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + var filter *diffTypeFilter + if tt.filterType != "" { + filter = newDiffTypeFilter(tt.filterType) + } + dArgs := &diffArgs{ diffDisplaySettings: &diffDisplaySettings{ - filter: tt.filter, + filter: filter, }, } tableSummary := diff.TableDeltaSummary{ @@ -495,11 +388,15 @@ func (m *mockRowWriter) Close(_ /* ctx */ context.Context) error { func TestLazyRowWriter_NoRowsWritten(t *testing.T) { mockDW := &mockDiffWriter{} - factory := func() (diff.SqlRowDiffWriter, error) { - return mockDW.RowWriter(nil, nil, diff.TableDeltaSummary{}, nil) + realWriter := &mockRowWriter{} + + beginTableCalled := false + onFirstWrite := func() error { + beginTableCalled = true + return mockDW.BeginTable("fromTable", "toTable", false, false) } - lazyWriter := newLazyRowWriter(mockDW, "fromTable", "toTable", false, false, factory) + lazyWriter := newLazyRowWriter(realWriter, onFirstWrite) // Close without writing any rows err := lazyWriter.Close(context.Background()) @@ -508,18 +405,20 @@ func TestLazyRowWriter_NoRowsWritten(t *testing.T) { } // BeginTable should NEVER have been called - if mockDW.beginTableCalled { + if beginTableCalled { t.Error("BeginTable() was called even though no rows were written - should have been lazy!") } } func TestLazyRowWriter_RowsWritten(t *testing.T) { mockDW := &mockDiffWriter{} - factory := func() (diff.SqlRowDiffWriter, error) { - return mockDW.RowWriter(nil, nil, diff.TableDeltaSummary{}, nil) + realWriter := &mockRowWriter{} + + onFirstWrite := func() error { + return mockDW.BeginTable("fromTable", "toTable", false, false) } - lazyWriter := newLazyRowWriter(mockDW, "fromTable", "toTable", false, false, factory) + lazyWriter := newLazyRowWriter(realWriter, onFirstWrite) // Write a row ctx := sql.NewEmptyContext() @@ -542,11 +441,13 @@ func TestLazyRowWriter_RowsWritten(t *testing.T) { func TestLazyRowWriter_CombinedRowsWritten(t *testing.T) { mockDW := &mockDiffWriter{} - factory := func() (diff.SqlRowDiffWriter, error) { - return mockDW.RowWriter(nil, nil, diff.TableDeltaSummary{}, nil) + realWriter := &mockRowWriter{} + + onFirstWrite := func() error { + return mockDW.BeginTable("fromTable", "toTable", false, false) } - lazyWriter := newLazyRowWriter(mockDW, "fromTable", "toTable", false, false, factory) + lazyWriter := newLazyRowWriter(realWriter, onFirstWrite) // Write a combined row ctx := sql.NewEmptyContext() @@ -564,24 +465,23 @@ func TestLazyRowWriter_CombinedRowsWritten(t *testing.T) { func TestLazyRowWriter_InitializedOnlyOnce(t *testing.T) { callCount := 0 mockDW := &mockDiffWriter{} - factory := func() (diff.SqlRowDiffWriter, error) { - return mockDW.RowWriter(nil, nil, diff.TableDeltaSummary{}, nil) + realWriter := &mockRowWriter{} + + onFirstWrite := func() error { + callCount++ + return mockDW.BeginTable("fromTable", "toTable", false, false) } - lazyWriter := newLazyRowWriter(mockDW, "fromTable", "toTable", false, false, factory) + lazyWriter := newLazyRowWriter(realWriter, onFirstWrite) ctx := sql.NewEmptyContext() // Write multiple rows for i := 0; i < 5; i++ { - mockDW.beginTableCalled = false // Reset flag err := lazyWriter.WriteRow(ctx, sql.Row{}, diff.Added, []diff.ChangeType{}) if err != nil { t.Fatalf("WriteRow() %d returned error: %v", i, err) } - if mockDW.beginTableCalled { - callCount++ - } } // BeginTable should have been called exactly ONCE (on first write only) @@ -589,3 +489,48 @@ func TestLazyRowWriter_InitializedOnlyOnce(t *testing.T) { t.Errorf("BeginTable() called %d times, expected exactly 1", callCount) } } + +func TestShouldSkipRow(t *testing.T) { + tests := []struct { + name string + filterType string + rowChangeType diff.ChangeType + expectedResult bool + }{ + {"filter=added, row=Added", diff.DiffTypeAdded, diff.Added, false}, + {"filter=added, row=Removed", diff.DiffTypeAdded, diff.Removed, true}, + {"filter=added, row=ModifiedOld", diff.DiffTypeAdded, diff.ModifiedOld, true}, + {"filter=added, row=ModifiedNew", diff.DiffTypeAdded, diff.ModifiedNew, true}, + + {"filter=removed, row=Added", diff.DiffTypeRemoved, diff.Added, true}, + {"filter=removed, row=Removed", diff.DiffTypeRemoved, diff.Removed, false}, + {"filter=removed, row=ModifiedOld", diff.DiffTypeRemoved, diff.ModifiedOld, true}, + + {"filter=modified, row=Added", diff.DiffTypeModified, diff.Added, true}, + {"filter=modified, row=Removed", diff.DiffTypeModified, diff.Removed, true}, + {"filter=modified, row=ModifiedOld", diff.DiffTypeModified, diff.ModifiedOld, false}, + {"filter=modified, row=ModifiedNew", diff.DiffTypeModified, diff.ModifiedNew, false}, + + {"filter=all, row=Added", diff.DiffTypeAll, diff.Added, false}, + {"filter=all, row=Removed", diff.DiffTypeAll, diff.Removed, false}, + {"filter=all, row=ModifiedOld", diff.DiffTypeAll, diff.ModifiedOld, false}, + + {"nil filter, row=Added", "", diff.Added, false}, + {"nil filter, row=Removed", "", diff.Removed, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var filter *diffTypeFilter + if tt.filterType != "" { + filter = newDiffTypeFilter(tt.filterType) + } + + result := shouldSkipRow(filter, tt.rowChangeType) + + if result != tt.expectedResult { + t.Errorf("expected %v, got %v", tt.expectedResult, result) + } + }) + } +} diff --git a/go/cmd/dolt/commands/merge.go b/go/cmd/dolt/commands/merge.go index 6d39ccad69..2c5f834f32 100644 --- a/go/cmd/dolt/commands/merge.go +++ b/go/cmd/dolt/commands/merge.go @@ -480,17 +480,17 @@ func calculateMergeStats(queryist cli.Queryist, sqlCtx *sql.Context, mergeStats if strings.HasPrefix(summary.TableName.Name, diff.DBPrefix) { continue } - if summary.DiffType == "added" { + if summary.DiffType == diff.DiffTypeAdded { allUnmodified = false mergeStats[summary.TableName.Name] = &merge.MergeStats{ Operation: merge.TableAdded, } - } else if summary.DiffType == "dropped" { + } else if summary.DiffType == diff.DiffTypeRemoved { allUnmodified = false mergeStats[summary.TableName.Name] = &merge.MergeStats{ Operation: merge.TableRemoved, } - } else if summary.DiffType == "modified" || summary.DiffType == "renamed" { + } else if summary.DiffType == diff.DiffTypeModified { allUnmodified = false mergeStats[summary.TableName.Name] = &merge.MergeStats{ Operation: merge.TableModified, diff --git a/go/libraries/doltcore/diff/table_deltas.go b/go/libraries/doltcore/diff/table_deltas.go index 60728941f2..1c26b257ac 100644 --- a/go/libraries/doltcore/diff/table_deltas.go +++ b/go/libraries/doltcore/diff/table_deltas.go @@ -715,7 +715,7 @@ func (td TableDelta) GetSummary(ctx context.Context) (*TableDeltaSummary, error) FromTableName: td.FromName, DataChange: dataChange, SchemaChange: true, - DiffType: "dropped", + DiffType: DiffTypeRemoved, }, nil } @@ -726,7 +726,7 @@ func (td TableDelta) GetSummary(ctx context.Context) (*TableDeltaSummary, error) ToTableName: td.ToName, DataChange: dataChange, SchemaChange: true, - DiffType: "added", + DiffType: DiffTypeAdded, }, nil } @@ -738,7 +738,7 @@ func (td TableDelta) GetSummary(ctx context.Context) (*TableDeltaSummary, error) ToTableName: td.ToName, DataChange: dataChange, SchemaChange: true, - DiffType: "renamed", + DiffType: DiffTypeModified, }, nil } @@ -753,7 +753,7 @@ func (td TableDelta) GetSummary(ctx context.Context) (*TableDeltaSummary, error) ToTableName: td.ToName, DataChange: dataChange, SchemaChange: schemaChange, - DiffType: "modified", + DiffType: DiffTypeModified, }, nil } From 3c9fb41e10fa1ff72fc4f095c90e3754ae10d2fe Mon Sep 17 00:00:00 2001 From: David Dansby <39511285+codeaucafe@users.noreply.github.com> Date: Fri, 28 Nov 2025 19:29:44 -0800 Subject: [PATCH 07/22] refactor(diff): add renamed filter and removed alias Add DiffTypeRenamed constant and "removed" as user-friendly alias for "dropped" filter option. This provides more granular filtering and improves user experience with familiar terminology. List of changes: - Add DiffTypeRenamed constant for renamed tables - Revert GetSummary to use DiffTypeRenamed instead of treating renames as modified (table_deltas.go:742) - Add "removed" as alias that maps to "dropped" internally for user convenience - Update filter validation to include renamed option - Update CLI help text to document all filter options including renamed and removed alias - Handle renamed tables in merge stats alongside modified - Update getDiffSummariesBetweenRefs and getSchemaDiffSummariesBetweenRefs to handle renamed diff type - Update all tests to use new constants and test renamed filtering Filter options now available: added, modified, renamed, dropped, removed (alias for dropped). Refs: #1430 --- go/cmd/dolt/commands/diff.go | 33 +++++++----- go/cmd/dolt/commands/diff_filter_test.go | 62 +++++++++++----------- go/cmd/dolt/commands/merge.go | 4 +- go/libraries/doltcore/diff/table_deltas.go | 9 ++-- 4 files changed, 57 insertions(+), 51 deletions(-) diff --git a/go/cmd/dolt/commands/diff.go b/go/cmd/dolt/commands/diff.go index 6365714070..f739ae9d9e 100644 --- a/go/cmd/dolt/commands/diff.go +++ b/go/cmd/dolt/commands/diff.go @@ -87,7 +87,7 @@ The diffs displayed can be limited to show the first N by providing the paramete To filter which data rows are displayed, use {{.EmphasisLeft}}--where {{.EmphasisRight}}. Table column names in the filter expression must be prefixed with {{.EmphasisLeft}}from_{{.EmphasisRight}} or {{.EmphasisLeft}}to_{{.EmphasisRight}}, e.g. {{.EmphasisLeft}}to_COLUMN_NAME > 100{{.EmphasisRight}} or {{.EmphasisLeft}}from_COLUMN_NAME + to_COLUMN_NAME = 0{{.EmphasisRight}}. -To filter diff output by change type, use {{.EmphasisLeft}}--filter {{.EmphasisRight}} where {{.EmphasisLeft}}{{.EmphasisRight}} is one of {{.EmphasisLeft}}added{{.EmphasisRight}}, {{.EmphasisLeft}}modified{{.EmphasisRight}}, or {{.EmphasisLeft}}removed{{.EmphasisRight}}. The {{.EmphasisLeft}}added{{.EmphasisRight}} filter shows only additions (new tables or rows), {{.EmphasisLeft}}modified{{.EmphasisRight}} shows only modifications (schema changes, renames, or row updates), and {{.EmphasisLeft}}removed{{.EmphasisRight}} shows only deletions (dropped tables or deleted rows). For example, {{.EmphasisLeft}}dolt diff --filter=removed{{.EmphasisRight}} shows only deleted rows and dropped tables. +To filter diff output by change type, use {{.EmphasisLeft}}--filter {{.EmphasisRight}} where {{.EmphasisLeft}}{{.EmphasisRight}} is one of {{.EmphasisLeft}}added{{.EmphasisRight}}, {{.EmphasisLeft}}modified{{.EmphasisRight}}, {{.EmphasisLeft}}renamed{{.EmphasisRight}}, or {{.EmphasisLeft}}dropped{{.EmphasisRight}}. The {{.EmphasisLeft}}added{{.EmphasisRight}} filter shows only additions (new tables or rows), {{.EmphasisLeft}}modified{{.EmphasisRight}} shows only schema modifications or row updates, {{.EmphasisLeft}}renamed{{.EmphasisRight}} shows only renamed tables, and {{.EmphasisLeft}}dropped{{.EmphasisRight}} shows only deletions (dropped tables or deleted rows). You can also use {{.EmphasisLeft}}removed{{.EmphasisRight}} as an alias for {{.EmphasisLeft}}dropped{{.EmphasisRight}}. For example, {{.EmphasisLeft}}dolt diff --filter=dropped{{.EmphasisRight}} shows only deleted rows and dropped tables. The {{.EmphasisLeft}}--diff-mode{{.EmphasisRight}} argument controls how modified rows are presented when the format output is set to {{.EmphasisLeft}}tabular{{.EmphasisRight}}. When set to {{.EmphasisLeft}}row{{.EmphasisRight}}, modified rows are presented as old and new rows. When set to {{.EmphasisLeft}}line{{.EmphasisRight}}, modified rows are presented as a single row, and changes are presented using "+" and "-" within the column. When set to {{.EmphasisLeft}}in-place{{.EmphasisRight}}, modified rows are presented as a single row, and changes are presented side-by-side with a color distinction (requires a color-enabled terminal). When set to {{.EmphasisLeft}}context{{.EmphasisRight}}, rows that contain at least one column that spans multiple lines uses {{.EmphasisLeft}}line{{.EmphasisRight}}, while all other rows use {{.EmphasisLeft}}row{{.EmphasisRight}}. The default value is {{.EmphasisLeft}}context{{.EmphasisRight}}. `, @@ -144,14 +144,21 @@ type diffTypeFilter struct { // newDiffTypeFilter creates a filter for the specified diff type. // Pass diff.DiffTypeAll or empty string to include all types. +// Accepts "removed" as an alias for "dropped" for user convenience. func newDiffTypeFilter(filterType string) *diffTypeFilter { if filterType == "" || filterType == diff.DiffTypeAll { return &diffTypeFilter{filters: nil} // nil means include all } + // Map "removed" to "dropped" (alias for user convenience) + internalFilterType := filterType + if filterType == "removed" { + internalFilterType = diff.DiffTypeDropped + } + return &diffTypeFilter{ filters: map[string]bool{ - filterType: true, + internalFilterType: true, }, } } @@ -176,7 +183,8 @@ func (df *diffTypeFilter) isValid() bool { for filterType := range df.filters { if filterType != diff.DiffTypeAdded && filterType != diff.DiffTypeModified && - filterType != diff.DiffTypeRemoved { + filterType != diff.DiffTypeRenamed && + filterType != diff.DiffTypeDropped { return false } } @@ -355,8 +363,8 @@ func (cmd DiffCmd) validateArgs(apr *argparser.ArgParseResults) errhand.VerboseE if hasFilter { filter := newDiffTypeFilter(filterValue) if !filter.isValid() { - return errhand.BuildDError("invalid filter: %s. Valid values are: %s, %s, %s", - filterValue, diff.DiffTypeAdded, diff.DiffTypeModified, diff.DiffTypeRemoved).Build() + return errhand.BuildDError("invalid filter: %s. Valid values are: %s, %s, %s, %s (or %s)", + filterValue, diff.DiffTypeAdded, diff.DiffTypeModified, diff.DiffTypeRenamed, diff.DiffTypeDropped, "removed").Build() } } @@ -816,10 +824,10 @@ func getSchemaDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Contex diffType = diff.DiffTypeAdded tableName = toTable case toTable == "": - diffType = diff.DiffTypeRemoved + diffType = diff.DiffTypeDropped tableName = fromTable case fromTable != "" && toTable != "" && fromTable != toTable: - diffType = diff.DiffTypeModified // Renamed tables are treated as modified + diffType = diff.DiffTypeRenamed tableName = toTable default: return nil, fmt.Errorf("error: unexpected schema diff case: fromTable='%s', toTable='%s'", fromTable, toTable) @@ -881,17 +889,14 @@ func getDiffSummariesBetweenRefs(queryist cli.Queryist, sqlCtx *sql.Context, fro } switch summary.DiffType { - case diff.DiffTypeRemoved: + case diff.DiffTypeDropped: summary.TableName = summary.FromTableName case diff.DiffTypeAdded: summary.TableName = summary.ToTableName + case diff.DiffTypeRenamed: + summary.TableName = summary.ToTableName case diff.DiffTypeModified: - // For renamed tables, use ToTableName; for other modifications, use FromTableName - if summary.FromTableName.Name != summary.ToTableName.Name { - summary.TableName = summary.ToTableName - } else { - summary.TableName = summary.FromTableName - } + summary.TableName = summary.FromTableName default: return nil, fmt.Errorf("error: unexpected diff type '%s'", summary.DiffType) } diff --git a/go/cmd/dolt/commands/diff_filter_test.go b/go/cmd/dolt/commands/diff_filter_test.go index 51109ef8bb..8a6084e744 100644 --- a/go/cmd/dolt/commands/diff_filter_test.go +++ b/go/cmd/dolt/commands/diff_filter_test.go @@ -33,7 +33,7 @@ func TestDiffTypeFilter_IsValid(t *testing.T) { }{ {"valid: added", diff.DiffTypeAdded, true}, {"valid: modified", diff.DiffTypeModified, true}, - {"valid: removed", diff.DiffTypeRemoved, true}, + {"valid: removed", diff.DiffTypeDropped, true}, {"valid: all", diff.DiffTypeAll, true}, {"invalid: empty string with nil filter", "", true}, // nil filter is valid {"invalid: random string", "invalid", false}, @@ -68,27 +68,27 @@ func TestDiffTypeFilter_ShouldInclude(t *testing.T) { // Testing with filter=added {"filter=added, check added", diff.DiffTypeAdded, diff.DiffTypeAdded, true}, {"filter=added, check modified", diff.DiffTypeAdded, diff.DiffTypeModified, false}, - {"filter=added, check removed", diff.DiffTypeAdded, diff.DiffTypeRemoved, false}, + {"filter=added, check removed", diff.DiffTypeAdded, diff.DiffTypeDropped, false}, // Testing with filter=modified {"filter=modified, check added", diff.DiffTypeModified, diff.DiffTypeAdded, false}, {"filter=modified, check modified", diff.DiffTypeModified, diff.DiffTypeModified, true}, - {"filter=modified, check removed", diff.DiffTypeModified, diff.DiffTypeRemoved, false}, + {"filter=modified, check removed", diff.DiffTypeModified, diff.DiffTypeDropped, false}, - // Testing with filter=removed - {"filter=removed, check added", diff.DiffTypeRemoved, diff.DiffTypeAdded, false}, - {"filter=removed, check modified", diff.DiffTypeRemoved, diff.DiffTypeModified, false}, - {"filter=removed, check removed", diff.DiffTypeRemoved, diff.DiffTypeRemoved, true}, + // Testing with filter=dropped + {"filter=dropped, check added", diff.DiffTypeDropped, diff.DiffTypeAdded, false}, + {"filter=dropped, check modified", diff.DiffTypeDropped, diff.DiffTypeModified, false}, + {"filter=dropped, check removed", diff.DiffTypeDropped, diff.DiffTypeDropped, true}, // Testing with filter=all {"filter=all, check added", diff.DiffTypeAll, diff.DiffTypeAdded, true}, {"filter=all, check modified", diff.DiffTypeAll, diff.DiffTypeModified, true}, - {"filter=all, check removed", diff.DiffTypeAll, diff.DiffTypeRemoved, true}, + {"filter=all, check removed", diff.DiffTypeAll, diff.DiffTypeDropped, true}, // Testing with empty filter (nil filters map) {"filter=empty, check added", "", diff.DiffTypeAdded, true}, {"filter=empty, check modified", "", diff.DiffTypeModified, true}, - {"filter=empty, check removed", "", diff.DiffTypeRemoved, true}, + {"filter=empty, check removed", "", diff.DiffTypeDropped, true}, } for _, tt := range tests { @@ -110,7 +110,7 @@ func TestDiffTypeFilter_ConsistencyAcrossMethods(t *testing.T) { if !df.shouldInclude(diff.DiffTypeAdded) { t.Error("filter=all should include added") } - if !df.shouldInclude(diff.DiffTypeRemoved) { + if !df.shouldInclude(diff.DiffTypeDropped) { t.Error("filter=all should include removed") } if !df.shouldInclude(diff.DiffTypeModified) { @@ -125,7 +125,7 @@ func TestDiffTypeFilter_ConsistencyAcrossMethods(t *testing.T) { if !df.shouldInclude(diff.DiffTypeAdded) { t.Error("filter=added should include added") } - if df.shouldInclude(diff.DiffTypeRemoved) { + if df.shouldInclude(diff.DiffTypeDropped) { t.Error("filter=added should not include removed") } if df.shouldInclude(diff.DiffTypeModified) { @@ -133,17 +133,17 @@ func TestDiffTypeFilter_ConsistencyAcrossMethods(t *testing.T) { } }) - t.Run("filter=removed only includes removed", func(t *testing.T) { - df := newDiffTypeFilter(diff.DiffTypeRemoved) + t.Run("filter=dropped only includes removed", func(t *testing.T) { + df := newDiffTypeFilter(diff.DiffTypeDropped) if df.shouldInclude(diff.DiffTypeAdded) { - t.Error("filter=removed should not include added") + t.Error("filter=dropped should not include added") } - if !df.shouldInclude(diff.DiffTypeRemoved) { - t.Error("filter=removed should include removed") + if !df.shouldInclude(diff.DiffTypeDropped) { + t.Error("filter=dropped should include removed") } if df.shouldInclude(diff.DiffTypeModified) { - t.Error("filter=removed should not include modified") + t.Error("filter=dropped should not include modified") } }) @@ -153,7 +153,7 @@ func TestDiffTypeFilter_ConsistencyAcrossMethods(t *testing.T) { if df.shouldInclude(diff.DiffTypeAdded) { t.Error("filter=modified should not include added") } - if df.shouldInclude(diff.DiffTypeRemoved) { + if df.shouldInclude(diff.DiffTypeDropped) { t.Error("filter=modified should not include removed") } if !df.shouldInclude(diff.DiffTypeModified) { @@ -186,7 +186,7 @@ func TestFilterConstants(t *testing.T) { }{ {"DiffTypeAdded value", diff.DiffTypeAdded, "added"}, {"DiffTypeModified value", diff.DiffTypeModified, "modified"}, - {"DiffTypeRemoved value", diff.DiffTypeRemoved, "removed"}, + {"DiffTypeDropped value", diff.DiffTypeDropped, "dropped"}, {"DiffTypeAll value", diff.DiffTypeAll, "all"}, } @@ -201,7 +201,7 @@ func TestFilterConstants(t *testing.T) { func TestFilterConstants_AreUnique(t *testing.T) { // Test that all filter constants are unique - constants := []string{diff.DiffTypeAdded, diff.DiffTypeModified, diff.DiffTypeRemoved, diff.DiffTypeAll} + constants := []string{diff.DiffTypeAdded, diff.DiffTypeModified, diff.DiffTypeDropped, diff.DiffTypeAll} seen := make(map[string]bool) for _, c := range constants { @@ -218,7 +218,7 @@ func TestFilterConstants_AreUnique(t *testing.T) { func TestFilterConstants_AreLowercase(t *testing.T) { // Test that filter constants are lowercase (convention) - constants := []string{diff.DiffTypeAdded, diff.DiffTypeModified, diff.DiffTypeRemoved, diff.DiffTypeAll} + constants := []string{diff.DiffTypeAdded, diff.DiffTypeModified, diff.DiffTypeDropped, diff.DiffTypeAll} for _, c := range constants { if c != strings.ToLower(c) { @@ -265,7 +265,7 @@ func TestShouldUseLazyHeader(t *testing.T) { }, { name: "don't use lazy: table renamed", - filterType: diff.DiffTypeRemoved, + filterType: diff.DiffTypeDropped, schemaChange: false, isRename: true, expectedResult: false, @@ -285,8 +285,8 @@ func TestShouldUseLazyHeader(t *testing.T) { expectedResult: true, }, { - name: "use lazy: filter=removed, data-only", - filterType: diff.DiffTypeRemoved, + name: "use lazy: filter=dropped, data-only", + filterType: diff.DiffTypeDropped, schemaChange: false, isRename: false, expectedResult: true, @@ -498,25 +498,25 @@ func TestShouldSkipRow(t *testing.T) { expectedResult bool }{ {"filter=added, row=Added", diff.DiffTypeAdded, diff.Added, false}, - {"filter=added, row=Removed", diff.DiffTypeAdded, diff.Removed, true}, + {"filter=added, row=Dropped", diff.DiffTypeAdded, diff.Removed, true}, {"filter=added, row=ModifiedOld", diff.DiffTypeAdded, diff.ModifiedOld, true}, {"filter=added, row=ModifiedNew", diff.DiffTypeAdded, diff.ModifiedNew, true}, - {"filter=removed, row=Added", diff.DiffTypeRemoved, diff.Added, true}, - {"filter=removed, row=Removed", diff.DiffTypeRemoved, diff.Removed, false}, - {"filter=removed, row=ModifiedOld", diff.DiffTypeRemoved, diff.ModifiedOld, true}, + {"filter=dropped, row=Added", diff.DiffTypeDropped, diff.Added, true}, + {"filter=dropped, row=Dropped", diff.DiffTypeDropped, diff.Removed, false}, + {"filter=dropped, row=ModifiedOld", diff.DiffTypeDropped, diff.ModifiedOld, true}, {"filter=modified, row=Added", diff.DiffTypeModified, diff.Added, true}, - {"filter=modified, row=Removed", diff.DiffTypeModified, diff.Removed, true}, + {"filter=modified, row=Dropped", diff.DiffTypeModified, diff.Removed, true}, {"filter=modified, row=ModifiedOld", diff.DiffTypeModified, diff.ModifiedOld, false}, {"filter=modified, row=ModifiedNew", diff.DiffTypeModified, diff.ModifiedNew, false}, {"filter=all, row=Added", diff.DiffTypeAll, diff.Added, false}, - {"filter=all, row=Removed", diff.DiffTypeAll, diff.Removed, false}, + {"filter=all, row=Dropped", diff.DiffTypeAll, diff.Removed, false}, {"filter=all, row=ModifiedOld", diff.DiffTypeAll, diff.ModifiedOld, false}, {"nil filter, row=Added", "", diff.Added, false}, - {"nil filter, row=Removed", "", diff.Removed, false}, + {"nil filter, row=Dropped", "", diff.Removed, false}, } for _, tt := range tests { diff --git a/go/cmd/dolt/commands/merge.go b/go/cmd/dolt/commands/merge.go index 2c5f834f32..a2b0dcc73e 100644 --- a/go/cmd/dolt/commands/merge.go +++ b/go/cmd/dolt/commands/merge.go @@ -485,12 +485,12 @@ func calculateMergeStats(queryist cli.Queryist, sqlCtx *sql.Context, mergeStats mergeStats[summary.TableName.Name] = &merge.MergeStats{ Operation: merge.TableAdded, } - } else if summary.DiffType == diff.DiffTypeRemoved { + } else if summary.DiffType == diff.DiffTypeDropped { allUnmodified = false mergeStats[summary.TableName.Name] = &merge.MergeStats{ Operation: merge.TableRemoved, } - } else if summary.DiffType == diff.DiffTypeModified { + } else if summary.DiffType == diff.DiffTypeModified || summary.DiffType == diff.DiffTypeRenamed { allUnmodified = false mergeStats[summary.TableName.Name] = &merge.MergeStats{ Operation: merge.TableModified, diff --git a/go/libraries/doltcore/diff/table_deltas.go b/go/libraries/doltcore/diff/table_deltas.go index 1c26b257ac..1809297220 100644 --- a/go/libraries/doltcore/diff/table_deltas.go +++ b/go/libraries/doltcore/diff/table_deltas.go @@ -45,7 +45,8 @@ const ( const ( DiffTypeAdded = "added" DiffTypeModified = "modified" - DiffTypeRemoved = "removed" + DiffTypeRenamed = "renamed" + DiffTypeDropped = "dropped" DiffTypeAll = "all" ) @@ -114,7 +115,7 @@ func ChangeTypeToDiffType(ct ChangeType) string { case Added: return DiffTypeAdded case Removed: - return DiffTypeRemoved + return DiffTypeDropped case ModifiedOld, ModifiedNew: // Both ModifiedOld and ModifiedNew represent the same logical change: modified return DiffTypeModified @@ -715,7 +716,7 @@ func (td TableDelta) GetSummary(ctx context.Context) (*TableDeltaSummary, error) FromTableName: td.FromName, DataChange: dataChange, SchemaChange: true, - DiffType: DiffTypeRemoved, + DiffType: DiffTypeDropped, }, nil } @@ -738,7 +739,7 @@ func (td TableDelta) GetSummary(ctx context.Context) (*TableDeltaSummary, error) ToTableName: td.ToName, DataChange: dataChange, SchemaChange: true, - DiffType: DiffTypeModified, + DiffType: DiffTypeRenamed, }, nil } From b89f07b82cdb17733cfd97b2b56181cfac94ce28 Mon Sep 17 00:00:00 2001 From: David Dansby <39511285+codeaucafe@users.noreply.github.com> Date: Fri, 28 Nov 2025 19:47:31 -0800 Subject: [PATCH 08/22] test(diff): add tests for renamed filter and removed alias Add tests for the --filter=renamed option and the --filter=removed alias that maps to dropped. Go tests: - Tests for filter=renamed checking all diff types - Tests for "removed" alias mapping to dropped internally - Verify renamed filter only includes renamed tables BATS tests: - Test --filter=renamed with table rename scenario - Test --filter=dropped with table drop scenario - Verify --filter=removed alias works same as dropped - Verify other filters correctly exclude renamed/dropped tables Refs: #1430 --- go/cmd/dolt/commands/diff_filter_test.go | 14 ++++- integration-tests/bats/diff.bats | 69 ++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/go/cmd/dolt/commands/diff_filter_test.go b/go/cmd/dolt/commands/diff_filter_test.go index 8a6084e744..f3d705f2f0 100644 --- a/go/cmd/dolt/commands/diff_filter_test.go +++ b/go/cmd/dolt/commands/diff_filter_test.go @@ -78,7 +78,19 @@ func TestDiffTypeFilter_ShouldInclude(t *testing.T) { // Testing with filter=dropped {"filter=dropped, check added", diff.DiffTypeDropped, diff.DiffTypeAdded, false}, {"filter=dropped, check modified", diff.DiffTypeDropped, diff.DiffTypeModified, false}, - {"filter=dropped, check removed", diff.DiffTypeDropped, diff.DiffTypeDropped, true}, + {"filter=dropped, check dropped", diff.DiffTypeDropped, diff.DiffTypeDropped, true}, + {"filter=dropped, check renamed", diff.DiffTypeDropped, diff.DiffTypeRenamed, false}, + + // Testing with filter=renamed + {"filter=renamed, check added", diff.DiffTypeRenamed, diff.DiffTypeAdded, false}, + {"filter=renamed, check modified", diff.DiffTypeRenamed, diff.DiffTypeModified, false}, + {"filter=renamed, check dropped", diff.DiffTypeRenamed, diff.DiffTypeDropped, false}, + {"filter=renamed, check renamed", diff.DiffTypeRenamed, diff.DiffTypeRenamed, true}, + + // Testing with "removed" alias (should map to dropped) + {"filter=removed (alias), check dropped", "removed", diff.DiffTypeDropped, true}, + {"filter=removed (alias), check added", "removed", diff.DiffTypeAdded, false}, + {"filter=removed (alias), check renamed", "removed", diff.DiffTypeRenamed, false}, // Testing with filter=all {"filter=all, check added", diff.DiffTypeAll, diff.DiffTypeAdded, true}, diff --git a/integration-tests/bats/diff.bats b/integration-tests/bats/diff.bats index d2dad938a2..e920fd918b 100644 --- a/integration-tests/bats/diff.bats +++ b/integration-tests/bats/diff.bats @@ -2396,3 +2396,72 @@ EOF [ $status -eq 1 ] [[ $output =~ "invalid filter" ]] || false } + +@test "diff: --filter=renamed filters to only renamed tables" { + dolt sql -q "create table t(pk int primary key, val int)" + dolt sql -q "INSERT INTO t VALUES (1, 10)" + dolt add . && dolt commit -m "create table with data" + + # Rename the table + dolt sql -q "RENAME TABLE t TO t_renamed" + dolt add . && dolt commit -m "rename table" + + # filter=renamed should show the renamed table (shows different from/to names) + run dolt diff HEAD~1 --filter=renamed + [ $status -eq 0 ] + [[ $output =~ 'diff --dolt a/t b/t_renamed' ]] || false + [[ $output =~ '--- a/t' ]] || false + [[ $output =~ '+++ b/t_renamed' ]] || false + + # filter=added should not show the renamed table + run dolt diff HEAD~1 --filter=added + [ $status -eq 0 ] + [[ $output = '' ]] || false + + # filter=modified should not show the renamed table + run dolt diff HEAD~1 --filter=modified + [ $status -eq 0 ] + [[ $output = '' ]] || false + + # filter=dropped should not show the renamed table + run dolt diff HEAD~1 --filter=dropped + [ $status -eq 0 ] + [[ $output = '' ]] || false +} + +@test "diff: --filter=dropped filters to only dropped tables" { + dolt sql -q "create table t(pk int primary key, val int)" + dolt sql -q "INSERT INTO t VALUES (1, 10)" + dolt add . && dolt commit -m "create table with data" + + # Drop the table + dolt sql -q "DROP TABLE t" + dolt add . && dolt commit -m "drop table" + + # filter=dropped should show the dropped table + run dolt diff HEAD~1 --filter=dropped + [ $status -eq 0 ] + [[ $output =~ 'diff --dolt a/t b/t' ]] || false + [[ $output =~ 'deleted table' ]] || false + + # filter=removed (alias for dropped) should also show the dropped table + run dolt diff HEAD~1 --filter=removed + [ $status -eq 0 ] + [[ $output =~ 'diff --dolt a/t b/t' ]] || false + [[ $output =~ 'deleted table' ]] || false + + # filter=added should not show the dropped table + run dolt diff HEAD~1 --filter=added + [ $status -eq 0 ] + [[ $output = '' ]] || false + + # filter=modified should not show the dropped table + run dolt diff HEAD~1 --filter=modified + [ $status -eq 0 ] + [[ $output = '' ]] || false + + # filter=renamed should not show the dropped table + run dolt diff HEAD~1 --filter=renamed + [ $status -eq 0 ] + [[ $output = '' ]] || false +} From bd40e892fc754f6746e7c42bbbaee89582839466 Mon Sep 17 00:00:00 2001 From: David Dansby <39511285+codeaucafe@users.noreply.github.com> Date: Fri, 28 Nov 2025 19:53:16 -0800 Subject: [PATCH 09/22] docs(diff): update --filter CLI help text Update the short help text for the --filter parameter to document all valid filter options (added, modified, renamed, dropped) and mention that 'removed' is accepted as an alias for 'dropped'. Refs: #1430 --- go/cmd/dolt/cli/arg_parser_helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/cmd/dolt/cli/arg_parser_helpers.go b/go/cmd/dolt/cli/arg_parser_helpers.go index d706f0dc87..2bf10b18bd 100644 --- a/go/cmd/dolt/cli/arg_parser_helpers.go +++ b/go/cmd/dolt/cli/arg_parser_helpers.go @@ -311,7 +311,7 @@ func CreateDiffArgParser(isTableFunction bool) *argparser.ArgParser { ap.SupportsString(FormatFlag, "r", "result output format", "How to format diff output. Valid values are tabular, sql, json. Defaults to tabular.") ap.SupportsString(WhereParam, "", "column", "filters columns based on values in the diff. See {{.EmphasisLeft}}dolt diff --help{{.EmphasisRight}} for details.") ap.SupportsInt(LimitParam, "", "record_count", "limits to the first N diffs.") - ap.SupportsString(FilterParam, "", "diff_type", "filters results based on the type of modification (added, modified, removed).") + ap.SupportsString(FilterParam, "", "diff_type", "filters results based on the type of change (added, modified, renamed, dropped). 'removed' is accepted as an alias for 'dropped'.") ap.SupportsFlag(StagedFlag, "", "Show only the staged data changes.") ap.SupportsFlag(CachedFlag, "c", "Synonym for --staged") ap.SupportsFlag(MergeBase, "", "Uses merge base of the first commit and second commit (or HEAD if not supplied) as the first commit") From 3655b40fd0c553357e945b6b71697d23a7550835 Mon Sep 17 00:00:00 2001 From: elianddb Date: Wed, 3 Dec 2025 11:57:28 -0800 Subject: [PATCH 10/22] mv priv check to var --- .../doltcore/sqle/dprocedures/dolt_backup.go | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go b/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go index 2f03f88cec..827073d880 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go @@ -70,16 +70,8 @@ func doltBackup(ctx *sql.Context, args ...string) (sql.RowIter, error) { return nil, err } - if sqlserver.RunningInServerMode() { - // TODO(elianddb): DoltgreSQL needs an auth handler for stored procedures, i.e. AuthType_CALL, but for now we use - // this. dolt_backup already requires admin privilege on GMS due to its potentially destructive nature. - privileges, counter := ctx.GetPrivilegeSet() - if counter == 0 || !privileges.Has(sql.PrivilegeType_Super) { - return nil, sql.ErrPrivilegeCheckFailed.New(ctx.Session.Client().User) - } - if apr.ContainsAny(cli.AwsParams...) { - return nil, fmt.Errorf("AWS parameters are unavailable when running in server mode") - } + if sqlserver.RunningInServerMode() && apr.ContainsAny(cli.AwsParams...) { + return nil, fmt.Errorf("AWS parameters are unavailable when running in server mode") } if apr.NArg() == 0 || (apr.NArg() == 1 && apr.Contains(cli.VerboseFlag)) { @@ -397,3 +389,13 @@ func errDoltBackupUsage(funcParam string, requiredParams, optionalParams []strin return errors.New(builder.String()) } + +var UserHasSuperAccess = func(ctx *sql.Context) (bool, error) { + // TODO(elianddb): DoltgreSQL needs an auth handler for stored procedures, i.e. AuthType_CALL, but for now we use + // this. dolt_backup already requires admin privilege on GMS due to its potentially destructive nature. + privileges, counter := ctx.GetPrivilegeSet() + if counter == 0 || !privileges.Has(sql.PrivilegeType_Super) { + return false, sql.ErrPrivilegeCheckFailed.New(ctx.Session.Client().User) + } + return true, nil +} From 73428104fcab33702ca875f1677bcefc1a87e64b Mon Sep 17 00:00:00 2001 From: elianddb Date: Wed, 3 Dec 2025 12:50:40 -0800 Subject: [PATCH 11/22] rm UserHasSuperAccess and move to doltgresql --- go/libraries/doltcore/sqle/dprocedures/dolt_backup.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go b/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go index 827073d880..adb0feef60 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go @@ -389,13 +389,3 @@ func errDoltBackupUsage(funcParam string, requiredParams, optionalParams []strin return errors.New(builder.String()) } - -var UserHasSuperAccess = func(ctx *sql.Context) (bool, error) { - // TODO(elianddb): DoltgreSQL needs an auth handler for stored procedures, i.e. AuthType_CALL, but for now we use - // this. dolt_backup already requires admin privilege on GMS due to its potentially destructive nature. - privileges, counter := ctx.GetPrivilegeSet() - if counter == 0 || !privileges.Has(sql.PrivilegeType_Super) { - return false, sql.ErrPrivilegeCheckFailed.New(ctx.Session.Client().User) - } - return true, nil -} From d7cf4b6d3b200de6a8d2d7fea08944684b8f76aa Mon Sep 17 00:00:00 2001 From: elianddb Date: Fri, 5 Dec 2025 13:24:06 -0800 Subject: [PATCH 12/22] add dolt adapter to standardize table overwrites by integrators --- go/libraries/doltcore/doltdb/system_table.go | 5 --- .../doltcore/sqle/adapters/table_adapter.go | 26 ++++++++++++ go/libraries/doltcore/sqle/database.go | 10 ++++- .../doltcore/sqle/dtables/status_table.go | 42 +++++++++++++------ 4 files changed, 63 insertions(+), 20 deletions(-) create mode 100644 go/libraries/doltcore/sqle/adapters/table_adapter.go diff --git a/go/libraries/doltcore/doltdb/system_table.go b/go/libraries/doltcore/doltdb/system_table.go index 60ac6325c9..53932995ca 100644 --- a/go/libraries/doltcore/doltdb/system_table.go +++ b/go/libraries/doltcore/doltdb/system_table.go @@ -367,11 +367,6 @@ var GetSchemaConflictsTableName = func() string { return SchemaConflictsTableName } -// GetStatusTableName returns the status system table name. -var GetStatusTableName = func() string { - return StatusTableName -} - // GetTagsTableName returns the tags table name var GetTagsTableName = func() string { return TagsTableName diff --git a/go/libraries/doltcore/sqle/adapters/table_adapter.go b/go/libraries/doltcore/sqle/adapters/table_adapter.go new file mode 100644 index 0000000000..3fd999e2c0 --- /dev/null +++ b/go/libraries/doltcore/sqle/adapters/table_adapter.go @@ -0,0 +1,26 @@ +package adapters + +import ( + "github.com/dolthub/go-mysql-server/sql" + + "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" + "github.com/dolthub/dolt/go/libraries/doltcore/env" +) + +// TableAdapter provides a hook for extensions to customize or wrap table implementations. For example, this allows +// libraries like Doltgres to intercept system table creation and apply type conversions, schema modifications, or other +// customizations without modifying the core Dolt implementation for their compatibility. +type TableAdapter interface { + // CreateTable creates or wraps a system table. The function receives all necessary parameters to construct the + // table and can either build it from scratch or call the default Dolt constructor and wrap it. + CreateTable(ctx *sql.Context, tableName string, dDb *doltdb.DoltDB, workingSet *doltdb.WorkingSet, rootsProvider env.RootsProvider[*sql.Context]) sql.Table + + // TableName returns the preferred name for the adapter's table. This allows extensions to rename tables while + // preserving the underlying implementation. For example, Doltgres uses "status" while Dolt uses "dolt_status", + // enabling cleaner Postgres-style naming. + TableName() string +} + +// TableAdapters is a registry for TableAdapter implementations, keyed by table name. It is populated during package +// initialization and intended to be read-only thereafter. +var TableAdapters = make(map[string]TableAdapter) diff --git a/go/libraries/doltcore/sqle/database.go b/go/libraries/doltcore/sqle/database.go index 4196510389..37fd7ee137 100644 --- a/go/libraries/doltcore/sqle/database.go +++ b/go/libraries/doltcore/sqle/database.go @@ -44,6 +44,7 @@ import ( "github.com/dolthub/dolt/go/libraries/doltcore/rebase" "github.com/dolthub/dolt/go/libraries/doltcore/ref" "github.com/dolthub/dolt/go/libraries/doltcore/schema" + "github.com/dolthub/dolt/go/libraries/doltcore/sqle/adapters" "github.com/dolthub/dolt/go/libraries/doltcore/sqle/dprocedures" "github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess" "github.com/dolthub/dolt/go/libraries/doltcore/sqle/dtables" @@ -747,7 +748,7 @@ func (db Database) getTableInsensitiveWithRoot(ctx *sql.Context, head *doltdb.Co if !resolve.UseSearchPath || isDoltgresSystemTable { dt, found = dtables.NewCommitAncestorsTable(ctx, db.Name(), lwrName, db.ddb), true } - case doltdb.GetStatusTableName(), doltdb.StatusTableName: + case doltdb.StatusTableName, adapters.TableAdapters[doltdb.StatusTableName].TableName(): isDoltgresSystemTable, err := resolve.IsDoltgresSystemTable(ctx, tname, root) if err != nil { return nil, false, err @@ -804,7 +805,12 @@ func (db Database) getTableInsensitiveWithRoot(ctx *sql.Context, head *doltdb.Co } } - dt, found = dtables.NewStatusTable(ctx, lwrName, db.ddb, ws, rootsProvider), true + if tableAdapter, ok := adapters.TableAdapters[lwrName]; ok { + dt, found = tableAdapter.CreateTable(ctx, lwrName, db.ddb, ws, rootsProvider), true + } + if !ok { + return nil, false, sql.ErrTableNotFound.New(tblName) + } } case doltdb.MergeStatusTableName, doltdb.GetMergeStatusTableName(): isDoltgresSystemTable, err := resolve.IsDoltgresSystemTable(ctx, tname, root) diff --git a/go/libraries/doltcore/sqle/dtables/status_table.go b/go/libraries/doltcore/sqle/dtables/status_table.go index 2e7673e8c8..c927d4621d 100644 --- a/go/libraries/doltcore/sqle/dtables/status_table.go +++ b/go/libraries/doltcore/sqle/dtables/status_table.go @@ -25,11 +25,35 @@ import ( "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" "github.com/dolthub/dolt/go/libraries/doltcore/env" "github.com/dolthub/dolt/go/libraries/doltcore/schema" + "github.com/dolthub/dolt/go/libraries/doltcore/sqle/adapters" "github.com/dolthub/dolt/go/libraries/doltcore/sqle/index" ) const statusDefaultRowCount = 10 +func init() { + adapters.TableAdapters[doltdb.StatusTableName] = DoltStatusTableAdapter{} +} + +// DoltStatusTableAdapter implements the adapters.TableAdapter interface. It serves as the default dolt_status table +// implementation, applying zero modifications. This exists in case no adapters are initialized by the integrator, i.e. +// Doltgres. +type DoltStatusTableAdapter struct{} + +var _ adapters.TableAdapter = DoltStatusTableAdapter{} + +// CreateTable implements the adapters.TableAdapter interface. It returns the default constructor for the dolt_status +// table, applying zero modifications. +func (d DoltStatusTableAdapter) CreateTable(ctx *sql.Context, tableName string, dDb *doltdb.DoltDB, workingSet *doltdb.WorkingSet, rootsProvider env.RootsProvider[*sql.Context]) sql.Table { + return NewStatusTable(ctx, tableName, dDb, workingSet, rootsProvider) +} + +// TableName implements the adapters.TableAdapter interface. It returns the default name for the dolt_status table, +// applying zero modifications. +func (d DoltStatusTableAdapter) TableName() string { + return doltdb.StatusTableName +} + // StatusTable is a sql.Table implementation that implements a system table which shows the dolt branches type StatusTable struct { rootsProvider env.RootsProvider[*sql.Context] @@ -61,20 +85,12 @@ func (st StatusTable) String() string { return st.tableName } -func getDoltStatusSchema(tableName string) sql.Schema { - return []*sql.Column{ - {Name: "table_name", Type: types.Text, Source: tableName, PrimaryKey: true, Nullable: false}, - {Name: "staged", Type: types.Boolean, Source: tableName, PrimaryKey: true, Nullable: false}, - {Name: "status", Type: types.Text, Source: tableName, PrimaryKey: true, Nullable: false}, - } -} - -// GetDoltStatusSchema returns the schema of the dolt_status system table. This is used -// by Doltgres to update the dolt_status schema using Doltgres types. -var GetDoltStatusSchema = getDoltStatusSchema - func (st StatusTable) Schema() sql.Schema { - return GetDoltStatusSchema(st.tableName) + return []*sql.Column{ + {Name: "table_name", Type: types.Text, Source: doltdb.StatusTableName, PrimaryKey: true, Nullable: false}, + {Name: "staged", Type: types.Boolean, Source: doltdb.StatusTableName, PrimaryKey: true, Nullable: false}, + {Name: "status", Type: types.Text, Source: doltdb.StatusTableName, PrimaryKey: true, Nullable: false}, + } } func (st StatusTable) Collation() sql.CollationID { From 752db82b84c48667bda07817150700faa61369bb Mon Sep 17 00:00:00 2001 From: elianddb Date: Fri, 5 Dec 2025 16:32:41 -0800 Subject: [PATCH 13/22] add dolt table specific adapter registry for integrators --- go/libraries/doltcore/doltdb/system_table.go | 1 - .../doltcore/sqle/adapters/table_adapter.go | 45 +++++++++++++++++-- go/libraries/doltcore/sqle/database.go | 11 ++--- .../doltcore/sqle/dtables/status_table.go | 15 +++---- .../doltcore/sqle/resolve/system_tables.go | 5 +++ 5 files changed, 57 insertions(+), 20 deletions(-) diff --git a/go/libraries/doltcore/doltdb/system_table.go b/go/libraries/doltcore/doltdb/system_table.go index 53932995ca..8dd79b68b5 100644 --- a/go/libraries/doltcore/doltdb/system_table.go +++ b/go/libraries/doltcore/doltdb/system_table.go @@ -155,7 +155,6 @@ func GeneratedSystemTableNames() []string { GetTableOfTablesWithViolationsName(), GetCommitsTableName(), GetCommitAncestorsTableName(), - GetStatusTableName(), GetRemotesTableName(), GetHelpTableName(), GetBackupsTableName(), diff --git a/go/libraries/doltcore/sqle/adapters/table_adapter.go b/go/libraries/doltcore/sqle/adapters/table_adapter.go index 3fd999e2c0..44b8fb9ada 100644 --- a/go/libraries/doltcore/sqle/adapters/table_adapter.go +++ b/go/libraries/doltcore/sqle/adapters/table_adapter.go @@ -21,6 +21,45 @@ type TableAdapter interface { TableName() string } -// TableAdapters is a registry for TableAdapter implementations, keyed by table name. It is populated during package -// initialization and intended to be read-only thereafter. -var TableAdapters = make(map[string]TableAdapter) +var DoltTableAdapterRegistry = newDoltTableAdapterRegistry() + +// doltTableAdapterRegistry is a TableAdapter registry for Dolt tables, keyed by table name. It is populated during +// package initialization (in the Dolt table source) and intended to be read-only thereafter. +type doltTableAdapterRegistry struct { + Adapters map[string]TableAdapter + adapterAliases map[string]string +} + +// newDoltTableAdapterRegistry constructs Dolt table adapter registry with empty alias and adapter maps. +func newDoltTableAdapterRegistry() *doltTableAdapterRegistry { + return &doltTableAdapterRegistry{ + Adapters: make(map[string]TableAdapter), + adapterAliases: make(map[string]string), + } +} + +// AddAdapter adds a TableAdapter to the Dolt table adapter registry with optional |aliases| (alternative table name +// keys). An alias cannot exist as both an adapter and alias, if you provide an alias that is already in the adapter +// map, it will be dropped. This overriding behavior is typically used by integrators, i.e. Doltgres, to replace the +// original Dolt table. +func (as *doltTableAdapterRegistry) AddAdapter(tableName string, adapter TableAdapter, aliases ...string) { + for _, alias := range aliases { + as.adapterAliases[alias] = tableName + if _, ok := as.Adapters[alias]; ok { + delete(as.Adapters, alias) // We don't want this to show up in the catalog. + } + } + as.adapterAliases[tableName] = tableName + as.Adapters[tableName] = adapter +} + +// GetAdapter gets a Dolt TableAdapter mapped to |name|, which can be an alias or the table name. +func (as *doltTableAdapterRegistry) GetAdapter(name string) TableAdapter { + name = as.adapterAliases[name] + return as.Adapters[name] +} + +// GetTableName gets the Dolt table name mapped to |name|, which can be an alias or the table name. +func (as *doltTableAdapterRegistry) GetTableName(name string) string { + return as.adapterAliases[name] +} diff --git a/go/libraries/doltcore/sqle/database.go b/go/libraries/doltcore/sqle/database.go index 37fd7ee137..b5de773700 100644 --- a/go/libraries/doltcore/sqle/database.go +++ b/go/libraries/doltcore/sqle/database.go @@ -748,7 +748,7 @@ func (db Database) getTableInsensitiveWithRoot(ctx *sql.Context, head *doltdb.Co if !resolve.UseSearchPath || isDoltgresSystemTable { dt, found = dtables.NewCommitAncestorsTable(ctx, db.Name(), lwrName, db.ddb), true } - case doltdb.StatusTableName, adapters.TableAdapters[doltdb.StatusTableName].TableName(): + case doltdb.StatusTableName, adapters.DoltTableAdapterRegistry.GetTableName(doltdb.StatusTableName): isDoltgresSystemTable, err := resolve.IsDoltgresSystemTable(ctx, tname, root) if err != nil { return nil, false, err @@ -804,13 +804,8 @@ func (db Database) getTableInsensitiveWithRoot(ctx *sql.Context, head *doltdb.Co rootsProvider = nil } } - - if tableAdapter, ok := adapters.TableAdapters[lwrName]; ok { - dt, found = tableAdapter.CreateTable(ctx, lwrName, db.ddb, ws, rootsProvider), true - } - if !ok { - return nil, false, sql.ErrTableNotFound.New(tblName) - } + adapter := adapters.DoltTableAdapterRegistry.GetAdapter(lwrName) + dt, found = adapter.CreateTable(ctx, lwrName, db.ddb, ws, rootsProvider), true } case doltdb.MergeStatusTableName, doltdb.GetMergeStatusTableName(): isDoltgresSystemTable, err := resolve.IsDoltgresSystemTable(ctx, tname, root) diff --git a/go/libraries/doltcore/sqle/dtables/status_table.go b/go/libraries/doltcore/sqle/dtables/status_table.go index c927d4621d..e787d37618 100644 --- a/go/libraries/doltcore/sqle/dtables/status_table.go +++ b/go/libraries/doltcore/sqle/dtables/status_table.go @@ -32,24 +32,23 @@ import ( const statusDefaultRowCount = 10 func init() { - adapters.TableAdapters[doltdb.StatusTableName] = DoltStatusTableAdapter{} + adapters.DoltTableAdapterRegistry.AddAdapter(doltdb.StatusTableName, DoltStatusTableAdapter{}) } -// DoltStatusTableAdapter implements the adapters.TableAdapter interface. It serves as the default dolt_status table -// implementation, applying zero modifications. This exists in case no adapters are initialized by the integrator, i.e. -// Doltgres. +// DoltStatusTableAdapter serves as the default [dtables.StatusTable] implementation, applying zero modifications. It +// exists in case no adapters are initialized by an integrator for the Dolt status table, i.e. go-mysql-server. +// +// DoltStatusTableAdapter implements the [adapters.TableAdapter] interface. type DoltStatusTableAdapter struct{} var _ adapters.TableAdapter = DoltStatusTableAdapter{} -// CreateTable implements the adapters.TableAdapter interface. It returns the default constructor for the dolt_status -// table, applying zero modifications. +// CreateTable returns the default constructor [NewStatusTable], applying zero modifications. func (d DoltStatusTableAdapter) CreateTable(ctx *sql.Context, tableName string, dDb *doltdb.DoltDB, workingSet *doltdb.WorkingSet, rootsProvider env.RootsProvider[*sql.Context]) sql.Table { return NewStatusTable(ctx, tableName, dDb, workingSet, rootsProvider) } -// TableName implements the adapters.TableAdapter interface. It returns the default name for the dolt_status table, -// applying zero modifications. +// TableName returns the default name for the dolt_status table. func (d DoltStatusTableAdapter) TableName() string { return doltdb.StatusTableName } diff --git a/go/libraries/doltcore/sqle/resolve/system_tables.go b/go/libraries/doltcore/sqle/resolve/system_tables.go index c37ecfc0af..c2c8448c12 100755 --- a/go/libraries/doltcore/sqle/resolve/system_tables.go +++ b/go/libraries/doltcore/sqle/resolve/system_tables.go @@ -19,6 +19,7 @@ import ( "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" "github.com/dolthub/dolt/go/libraries/doltcore/schema" + "github.com/dolthub/dolt/go/libraries/doltcore/sqle/adapters" ) // GetGeneratedSystemTables returns table names of all generated system tables. @@ -37,6 +38,10 @@ func GetGeneratedSystemTables(ctx context.Context, root doltdb.RootValue) ([]dol } } + for _, adapter := range adapters.DoltTableAdapterRegistry.Adapters { + s.Add(doltdb.TableName{Name: adapter.TableName(), Schema: doltdb.DoltNamespace}) + } + schemas, err := root.GetDatabaseSchemas(ctx) if err != nil { return nil, err From cd084b0cf1b2c9d950b1e81f6c494767787eacf6 Mon Sep 17 00:00:00 2001 From: elianddb Date: Mon, 8 Dec 2025 04:45:40 -0800 Subject: [PATCH 14/22] rm extra default adapter and normalize internal alias instead --- .../doltcore/sqle/adapters/table_adapter.go | 63 ++++++++++--------- go/libraries/doltcore/sqle/database.go | 8 +-- .../doltcore/sqle/dtables/status_table.go | 33 +++------- 3 files changed, 48 insertions(+), 56 deletions(-) diff --git a/go/libraries/doltcore/sqle/adapters/table_adapter.go b/go/libraries/doltcore/sqle/adapters/table_adapter.go index 44b8fb9ada..c41e423be0 100644 --- a/go/libraries/doltcore/sqle/adapters/table_adapter.go +++ b/go/libraries/doltcore/sqle/adapters/table_adapter.go @@ -11,9 +11,9 @@ import ( // libraries like Doltgres to intercept system table creation and apply type conversions, schema modifications, or other // customizations without modifying the core Dolt implementation for their compatibility. type TableAdapter interface { - // CreateTable creates or wraps a system table. The function receives all necessary parameters to construct the - // table and can either build it from scratch or call the default Dolt constructor and wrap it. - CreateTable(ctx *sql.Context, tableName string, dDb *doltdb.DoltDB, workingSet *doltdb.WorkingSet, rootsProvider env.RootsProvider[*sql.Context]) sql.Table + // NewTable creates or wraps a system table. The function receives all necessary parameters to construct the table + // and can either build it from scratch or call the default Dolt constructor and wrap it. + NewTable(ctx *sql.Context, tableName string, dDb *doltdb.DoltDB, workingSet *doltdb.WorkingSet, rootsProvider env.RootsProvider[*sql.Context]) sql.Table // TableName returns the preferred name for the adapter's table. This allows extensions to rename tables while // preserving the underlying implementation. For example, Doltgres uses "status" while Dolt uses "dolt_status", @@ -23,43 +23,48 @@ type TableAdapter interface { var DoltTableAdapterRegistry = newDoltTableAdapterRegistry() -// doltTableAdapterRegistry is a TableAdapter registry for Dolt tables, keyed by table name. It is populated during -// package initialization (in the Dolt table source) and intended to be read-only thereafter. +// doltTableAdapterRegistry is a TableAdapter registry for Dolt tables, keyed by the table's original name. It is +// populated during package initialization by integrators and intended to be read-only thereafter. type doltTableAdapterRegistry struct { - Adapters map[string]TableAdapter - adapterAliases map[string]string + Adapters map[string]TableAdapter + internalAliases map[string]string } -// newDoltTableAdapterRegistry constructs Dolt table adapter registry with empty alias and adapter maps. +// newDoltTableAdapterRegistry constructs Dolt table adapter registry with empty internal alias and adapter maps. func newDoltTableAdapterRegistry() *doltTableAdapterRegistry { return &doltTableAdapterRegistry{ - Adapters: make(map[string]TableAdapter), - adapterAliases: make(map[string]string), + Adapters: make(map[string]TableAdapter), + internalAliases: make(map[string]string), } } -// AddAdapter adds a TableAdapter to the Dolt table adapter registry with optional |aliases| (alternative table name -// keys). An alias cannot exist as both an adapter and alias, if you provide an alias that is already in the adapter -// map, it will be dropped. This overriding behavior is typically used by integrators, i.e. Doltgres, to replace the -// original Dolt table. -func (as *doltTableAdapterRegistry) AddAdapter(tableName string, adapter TableAdapter, aliases ...string) { - for _, alias := range aliases { - as.adapterAliases[alias] = tableName - if _, ok := as.Adapters[alias]; ok { - delete(as.Adapters, alias) // We don't want this to show up in the catalog. - } +// AddAdapter adds a TableAdapter to the Dolt table adapter registry with optional |internalAliases| (integrators' +// alternative Dolt table name keys). +func (as *doltTableAdapterRegistry) AddAdapter(doltTable string, adapter TableAdapter, internalAliases ...string) { + for _, alias := range internalAliases { + as.internalAliases[alias] = doltTable } - as.adapterAliases[tableName] = tableName - as.Adapters[tableName] = adapter + as.Adapters[doltTable] = adapter } -// GetAdapter gets a Dolt TableAdapter mapped to |name|, which can be an alias or the table name. -func (as *doltTableAdapterRegistry) GetAdapter(name string) TableAdapter { - name = as.adapterAliases[name] - return as.Adapters[name] +// GetAdapter gets a Dolt TableAdapter mapped to |name|, which can be the dolt table name or internal alias. +func (as *doltTableAdapterRegistry) GetAdapter(name string) (TableAdapter, bool) { + adapter, ok := as.Adapters[name] + if !ok { + name = as.internalAliases[name] + adapter, ok = as.Adapters[name] + } + + return adapter, ok } -// GetTableName gets the Dolt table name mapped to |name|, which can be an alias or the table name. -func (as *doltTableAdapterRegistry) GetTableName(name string) string { - return as.adapterAliases[name] +// NormalizeName normalizes |name| if it's an internal alias to the correct Dolt table name, if not match is found the +// |name| is returned as is. +func (as *doltTableAdapterRegistry) NormalizeName(name string) string { + doltTableName, ok := as.internalAliases[name] + if !ok { + return name + } + + return doltTableName } diff --git a/go/libraries/doltcore/sqle/database.go b/go/libraries/doltcore/sqle/database.go index b5de773700..24597295d5 100644 --- a/go/libraries/doltcore/sqle/database.go +++ b/go/libraries/doltcore/sqle/database.go @@ -619,7 +619,7 @@ func (db Database) getTableInsensitiveWithRoot(ctx *sql.Context, head *doltdb.Co var dt sql.Table found := false tname := doltdb.TableName{Name: lwrName, Schema: db.schemaName} - switch lwrName { + switch adapters.DoltTableAdapterRegistry.NormalizeName(lwrName) { case doltdb.GetLogTableName(), doltdb.LogTableName: isDoltgresSystemTable, err := resolve.IsDoltgresSystemTable(ctx, tname, root) if err != nil { @@ -748,7 +748,7 @@ func (db Database) getTableInsensitiveWithRoot(ctx *sql.Context, head *doltdb.Co if !resolve.UseSearchPath || isDoltgresSystemTable { dt, found = dtables.NewCommitAncestorsTable(ctx, db.Name(), lwrName, db.ddb), true } - case doltdb.StatusTableName, adapters.DoltTableAdapterRegistry.GetTableName(doltdb.StatusTableName): + case doltdb.StatusTableName: isDoltgresSystemTable, err := resolve.IsDoltgresSystemTable(ctx, tname, root) if err != nil { return nil, false, err @@ -804,8 +804,8 @@ func (db Database) getTableInsensitiveWithRoot(ctx *sql.Context, head *doltdb.Co rootsProvider = nil } } - adapter := adapters.DoltTableAdapterRegistry.GetAdapter(lwrName) - dt, found = adapter.CreateTable(ctx, lwrName, db.ddb, ws, rootsProvider), true + + dt, found = dtables.NewStatusTableWithAdapter(ctx, lwrName, db.ddb, ws, rootsProvider), true } case doltdb.MergeStatusTableName, doltdb.GetMergeStatusTableName(): isDoltgresSystemTable, err := resolve.IsDoltgresSystemTable(ctx, tname, root) diff --git a/go/libraries/doltcore/sqle/dtables/status_table.go b/go/libraries/doltcore/sqle/dtables/status_table.go index e787d37618..029b421278 100644 --- a/go/libraries/doltcore/sqle/dtables/status_table.go +++ b/go/libraries/doltcore/sqle/dtables/status_table.go @@ -31,28 +31,6 @@ import ( const statusDefaultRowCount = 10 -func init() { - adapters.DoltTableAdapterRegistry.AddAdapter(doltdb.StatusTableName, DoltStatusTableAdapter{}) -} - -// DoltStatusTableAdapter serves as the default [dtables.StatusTable] implementation, applying zero modifications. It -// exists in case no adapters are initialized by an integrator for the Dolt status table, i.e. go-mysql-server. -// -// DoltStatusTableAdapter implements the [adapters.TableAdapter] interface. -type DoltStatusTableAdapter struct{} - -var _ adapters.TableAdapter = DoltStatusTableAdapter{} - -// CreateTable returns the default constructor [NewStatusTable], applying zero modifications. -func (d DoltStatusTableAdapter) CreateTable(ctx *sql.Context, tableName string, dDb *doltdb.DoltDB, workingSet *doltdb.WorkingSet, rootsProvider env.RootsProvider[*sql.Context]) sql.Table { - return NewStatusTable(ctx, tableName, dDb, workingSet, rootsProvider) -} - -// TableName returns the default name for the dolt_status table. -func (d DoltStatusTableAdapter) TableName() string { - return doltdb.StatusTableName -} - // StatusTable is a sql.Table implementation that implements a system table which shows the dolt branches type StatusTable struct { rootsProvider env.RootsProvider[*sql.Context] @@ -104,7 +82,16 @@ func (st StatusTable) PartitionRows(context *sql.Context, _ sql.Partition) (sql. return newStatusItr(context, &st) } -// NewStatusTable creates a StatusTable +// NewStatusTableWithAdapter creates a StatusTable +func NewStatusTableWithAdapter(ctx *sql.Context, tableName string, ddb *doltdb.DoltDB, ws *doltdb.WorkingSet, rp env.RootsProvider[*sql.Context]) sql.Table { + adapter, ok := adapters.DoltTableAdapterRegistry.GetAdapter(tableName) + if ok { + return adapter.NewTable(ctx, tableName, ddb, ws, rp) + } + + return NewStatusTable(ctx, tableName, ddb, ws, rp) +} + func NewStatusTable(_ *sql.Context, tableName string, ddb *doltdb.DoltDB, ws *doltdb.WorkingSet, rp env.RootsProvider[*sql.Context]) sql.Table { return &StatusTable{ tableName: tableName, From 921d8a8909fe91f66b9f67e3e4cbeb71c0d3f130 Mon Sep 17 00:00:00 2001 From: elianddb Date: Mon, 8 Dec 2025 04:55:10 -0800 Subject: [PATCH 15/22] amend func docs --- go/libraries/doltcore/sqle/adapters/table_adapter.go | 2 +- go/libraries/doltcore/sqle/dtables/status_table.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/go/libraries/doltcore/sqle/adapters/table_adapter.go b/go/libraries/doltcore/sqle/adapters/table_adapter.go index c41e423be0..0a5b1e069d 100644 --- a/go/libraries/doltcore/sqle/adapters/table_adapter.go +++ b/go/libraries/doltcore/sqle/adapters/table_adapter.go @@ -58,7 +58,7 @@ func (as *doltTableAdapterRegistry) GetAdapter(name string) (TableAdapter, bool) return adapter, ok } -// NormalizeName normalizes |name| if it's an internal alias to the correct Dolt table name, if not match is found the +// NormalizeName normalizes |name| if it's an internal alias to the correct Dolt table name, if no match is found the // |name| is returned as is. func (as *doltTableAdapterRegistry) NormalizeName(name string) string { doltTableName, ok := as.internalAliases[name] diff --git a/go/libraries/doltcore/sqle/dtables/status_table.go b/go/libraries/doltcore/sqle/dtables/status_table.go index 029b421278..c9fff65125 100644 --- a/go/libraries/doltcore/sqle/dtables/status_table.go +++ b/go/libraries/doltcore/sqle/dtables/status_table.go @@ -82,7 +82,8 @@ func (st StatusTable) PartitionRows(context *sql.Context, _ sql.Partition) (sql. return newStatusItr(context, &st) } -// NewStatusTableWithAdapter creates a StatusTable +// NewStatusTableWithAdapter creates a new StatusTable using either an integrators' [adapters.TableAdapter] or the +// default [NewStatusTable] constructor. func NewStatusTableWithAdapter(ctx *sql.Context, tableName string, ddb *doltdb.DoltDB, ws *doltdb.WorkingSet, rp env.RootsProvider[*sql.Context]) sql.Table { adapter, ok := adapters.DoltTableAdapterRegistry.GetAdapter(tableName) if ok { @@ -92,6 +93,7 @@ func NewStatusTableWithAdapter(ctx *sql.Context, tableName string, ddb *doltdb.D return NewStatusTable(ctx, tableName, ddb, ws, rp) } +// NewStatusTable returns a new StatusTable. func NewStatusTable(_ *sql.Context, tableName string, ddb *doltdb.DoltDB, ws *doltdb.WorkingSet, rp env.RootsProvider[*sql.Context]) sql.Table { return &StatusTable{ tableName: tableName, From 6990d48af46a82ca56c8c83d736f0d2613210d05 Mon Sep 17 00:00:00 2001 From: elianddb Date: Mon, 8 Dec 2025 11:46:57 -0800 Subject: [PATCH 16/22] fix generated system table name set to use adapter names conditionally --- go/libraries/doltcore/doltdb/system_table.go | 2 ++ .../doltcore/sqle/adapters/table_adapter.go | 19 ++++++++-------- go/libraries/doltcore/sqle/database.go | 2 +- .../doltcore/sqle/dtables/status_table.go | 12 +++++----- .../doltcore/sqle/resolve/system_tables.go | 22 +++++++++---------- 5 files changed, 30 insertions(+), 27 deletions(-) diff --git a/go/libraries/doltcore/doltdb/system_table.go b/go/libraries/doltcore/doltdb/system_table.go index 8dd79b68b5..9701487a00 100644 --- a/go/libraries/doltcore/doltdb/system_table.go +++ b/go/libraries/doltcore/doltdb/system_table.go @@ -160,6 +160,8 @@ func GeneratedSystemTableNames() []string { GetBackupsTableName(), GetStashesTableName(), GetBranchActivityTableName(), + // [dtables.StatusTable] now uses [adapters.DoltTableAdapterRegistry] in its constructor for Doltgres. + StatusTableName, } } diff --git a/go/libraries/doltcore/sqle/adapters/table_adapter.go b/go/libraries/doltcore/sqle/adapters/table_adapter.go index 0a5b1e069d..12c1db1dad 100644 --- a/go/libraries/doltcore/sqle/adapters/table_adapter.go +++ b/go/libraries/doltcore/sqle/adapters/table_adapter.go @@ -23,8 +23,10 @@ type TableAdapter interface { var DoltTableAdapterRegistry = newDoltTableAdapterRegistry() -// doltTableAdapterRegistry is a TableAdapter registry for Dolt tables, keyed by the table's original name. It is -// populated during package initialization by integrators and intended to be read-only thereafter. +// doltTableAdapterRegistry is a Dolt table name to TableAdapter map. Integrators populate this registry during package +// initialization, and it's intended to be read-only thereafter. The registry links with existing Dolt system tables to +// allow them to be resolved and evaluated to integrator's version and internal aliases (integrators' Dolt table name +// keys). type doltTableAdapterRegistry struct { Adapters map[string]TableAdapter internalAliases map[string]string @@ -38,13 +40,12 @@ func newDoltTableAdapterRegistry() *doltTableAdapterRegistry { } } -// AddAdapter adds a TableAdapter to the Dolt table adapter registry with optional |internalAliases| (integrators' -// alternative Dolt table name keys). -func (as *doltTableAdapterRegistry) AddAdapter(doltTable string, adapter TableAdapter, internalAliases ...string) { +// AddAdapter maps |doltTableName| to an |adapter| in the Dolt table adapter registry, with optional |internalAliases|. +func (as *doltTableAdapterRegistry) AddAdapter(doltTableName string, adapter TableAdapter, internalAliases ...string) { for _, alias := range internalAliases { - as.internalAliases[alias] = doltTable + as.internalAliases[alias] = doltTableName } - as.Adapters[doltTable] = adapter + as.Adapters[doltTableName] = adapter } // GetAdapter gets a Dolt TableAdapter mapped to |name|, which can be the dolt table name or internal alias. @@ -58,8 +59,8 @@ func (as *doltTableAdapterRegistry) GetAdapter(name string) (TableAdapter, bool) return adapter, ok } -// NormalizeName normalizes |name| if it's an internal alias to the correct Dolt table name, if no match is found the -// |name| is returned as is. +// NormalizeName normalizes |name| if it's an internal alias of the underlying Dolt table name. If no match is found, +// |name| is returned as-is. func (as *doltTableAdapterRegistry) NormalizeName(name string) string { doltTableName, ok := as.internalAliases[name] if !ok { diff --git a/go/libraries/doltcore/sqle/database.go b/go/libraries/doltcore/sqle/database.go index 24597295d5..21265602a6 100644 --- a/go/libraries/doltcore/sqle/database.go +++ b/go/libraries/doltcore/sqle/database.go @@ -805,7 +805,7 @@ func (db Database) getTableInsensitiveWithRoot(ctx *sql.Context, head *doltdb.Co } } - dt, found = dtables.NewStatusTableWithAdapter(ctx, lwrName, db.ddb, ws, rootsProvider), true + dt, found = dtables.NewStatusTable(ctx, lwrName, db.ddb, ws, rootsProvider), true } case doltdb.MergeStatusTableName, doltdb.GetMergeStatusTableName(): isDoltgresSystemTable, err := resolve.IsDoltgresSystemTable(ctx, tname, root) diff --git a/go/libraries/doltcore/sqle/dtables/status_table.go b/go/libraries/doltcore/sqle/dtables/status_table.go index c9fff65125..fa39aa32ea 100644 --- a/go/libraries/doltcore/sqle/dtables/status_table.go +++ b/go/libraries/doltcore/sqle/dtables/status_table.go @@ -82,19 +82,19 @@ func (st StatusTable) PartitionRows(context *sql.Context, _ sql.Partition) (sql. return newStatusItr(context, &st) } -// NewStatusTableWithAdapter creates a new StatusTable using either an integrators' [adapters.TableAdapter] or the -// default [NewStatusTable] constructor. -func NewStatusTableWithAdapter(ctx *sql.Context, tableName string, ddb *doltdb.DoltDB, ws *doltdb.WorkingSet, rp env.RootsProvider[*sql.Context]) sql.Table { +// NewStatusTable creates a new StatusTable using either an integrators' [adapters.TableAdapter] or the default +// [NewStatusTableWithNoAdapter] constructor (Dolt table default implementation). +func NewStatusTable(ctx *sql.Context, tableName string, ddb *doltdb.DoltDB, ws *doltdb.WorkingSet, rp env.RootsProvider[*sql.Context]) sql.Table { adapter, ok := adapters.DoltTableAdapterRegistry.GetAdapter(tableName) if ok { return adapter.NewTable(ctx, tableName, ddb, ws, rp) } - return NewStatusTable(ctx, tableName, ddb, ws, rp) + return NewStatusTableWithNoAdapter(ctx, tableName, ddb, ws, rp) } -// NewStatusTable returns a new StatusTable. -func NewStatusTable(_ *sql.Context, tableName string, ddb *doltdb.DoltDB, ws *doltdb.WorkingSet, rp env.RootsProvider[*sql.Context]) sql.Table { +// NewStatusTableWithNoAdapter returns a new StatusTable. +func NewStatusTableWithNoAdapter(_ *sql.Context, tableName string, ddb *doltdb.DoltDB, ws *doltdb.WorkingSet, rp env.RootsProvider[*sql.Context]) sql.Table { return &StatusTable{ tableName: tableName, ddb: ddb, diff --git a/go/libraries/doltcore/sqle/resolve/system_tables.go b/go/libraries/doltcore/sqle/resolve/system_tables.go index c2c8448c12..3d8c777795 100755 --- a/go/libraries/doltcore/sqle/resolve/system_tables.go +++ b/go/libraries/doltcore/sqle/resolve/system_tables.go @@ -27,19 +27,19 @@ func GetGeneratedSystemTables(ctx context.Context, root doltdb.RootValue) ([]dol s := doltdb.NewTableNameSet(nil) // Depending on whether the search path is used, the generated system tables will either be in the dolt namespace - // or the empty (default) namespace - if !UseSearchPath { - for _, t := range doltdb.GeneratedSystemTableNames() { - s.Add(doltdb.TableName{Name: t}) + // or the empty (default) namespace. + for _, tableName := range doltdb.GeneratedSystemTableNames() { + adapter, ok := adapters.DoltTableAdapterRegistry.Adapters[tableName] + if ok { + tableName = adapter.TableName() } - } else { - for _, t := range doltdb.GeneratedSystemTableNames() { - s.Add(doltdb.TableName{Name: t, Schema: doltdb.DoltNamespace}) - } - } - for _, adapter := range adapters.DoltTableAdapterRegistry.Adapters { - s.Add(doltdb.TableName{Name: adapter.TableName(), Schema: doltdb.DoltNamespace}) + tableUnique := doltdb.TableName{Name: tableName} + if UseSearchPath { + tableUnique.Schema = doltdb.DoltNamespace + } + + s.Add(tableUnique) } schemas, err := root.GetDatabaseSchemas(ctx) From f9ff5ed52f081bb825e91d940ccdcb4883444df6 Mon Sep 17 00:00:00 2001 From: elianddb Date: Mon, 8 Dec 2025 12:01:03 -0800 Subject: [PATCH 17/22] add test for Dolt table adapter registry --- .../sqle/adapters/table_adapter_test.go | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 go/libraries/doltcore/sqle/adapters/table_adapter_test.go diff --git a/go/libraries/doltcore/sqle/adapters/table_adapter_test.go b/go/libraries/doltcore/sqle/adapters/table_adapter_test.go new file mode 100644 index 0000000000..1d63792c68 --- /dev/null +++ b/go/libraries/doltcore/sqle/adapters/table_adapter_test.go @@ -0,0 +1,64 @@ +package adapters + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/dolthub/go-mysql-server/sql" + + "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" + "github.com/dolthub/dolt/go/libraries/doltcore/env" +) + +type mockAdapter struct { + name string +} + +func (m mockAdapter) NewTable(_ *sql.Context, _ string, _ *doltdb.DoltDB, _ *doltdb.WorkingSet, _ env.RootsProvider[*sql.Context]) sql.Table { + return nil +} + +func (m mockAdapter) TableName() string { + return m.name +} + +func TestDoltTableAdapterRegistry(t *testing.T) { + registry := newDoltTableAdapterRegistry() + + statusAdapter := mockAdapter{name: "status"} + logAdapter := mockAdapter{name: "log"} + + registry.AddAdapter(doltdb.StatusTableName, statusAdapter, "status") + registry.AddAdapter(doltdb.LogTableName, logAdapter, "log") + + t.Run("GetAdapter", func(t *testing.T) { + adapter, ok := registry.GetAdapter("dolt_status") + require.True(t, ok) + require.Equal(t, "status", adapter.TableName()) + + adapter, ok = registry.GetAdapter("status") + require.True(t, ok) + require.Equal(t, "status", adapter.TableName()) + + _, ok = registry.GetAdapter("unknown_alias") + require.False(t, ok) + + _, ok = registry.GetAdapter("dolt_unknown") + require.False(t, ok) + }) + + t.Run("NormalizeName", func(t *testing.T) { + normalized := registry.NormalizeName("status") + require.Equal(t, "dolt_status", normalized) + + normalized = registry.NormalizeName("log") + require.Equal(t, "dolt_log", normalized) + + normalized = registry.NormalizeName("dolt_status") + require.Equal(t, "dolt_status", normalized) + + normalized = registry.NormalizeName("unknown_table") + require.Equal(t, "unknown_table", normalized) + }) +} From a17a3564b30c61ab75e85e1bbc608940b89179a8 Mon Sep 17 00:00:00 2001 From: elianddb Date: Mon, 8 Dec 2025 12:42:04 -0800 Subject: [PATCH 18/22] fix 'NewStatusTable' func doc --- go/libraries/doltcore/sqle/dtables/status_table.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/libraries/doltcore/sqle/dtables/status_table.go b/go/libraries/doltcore/sqle/dtables/status_table.go index fa39aa32ea..8213544981 100644 --- a/go/libraries/doltcore/sqle/dtables/status_table.go +++ b/go/libraries/doltcore/sqle/dtables/status_table.go @@ -82,8 +82,8 @@ func (st StatusTable) PartitionRows(context *sql.Context, _ sql.Partition) (sql. return newStatusItr(context, &st) } -// NewStatusTable creates a new StatusTable using either an integrators' [adapters.TableAdapter] or the default -// [NewStatusTableWithNoAdapter] constructor (Dolt table default implementation). +// NewStatusTable creates a new StatusTable using either an integrators' [adapters.TableAdapter] or the +// NewStatusTableWithNoAdapter constructor (the default implementation provided by Dolt). func NewStatusTable(ctx *sql.Context, tableName string, ddb *doltdb.DoltDB, ws *doltdb.WorkingSet, rp env.RootsProvider[*sql.Context]) sql.Table { adapter, ok := adapters.DoltTableAdapterRegistry.GetAdapter(tableName) if ok { From 16069d8889d6706d5f4f6329cc81dc63ca00bc13 Mon Sep 17 00:00:00 2001 From: elianddb Date: Mon, 8 Dec 2025 16:22:04 -0800 Subject: [PATCH 19/22] rename table_adapter.go -> table.go --- .../doltcore/sqle/adapters/{table_adapter.go => table.go} | 0 .../sqle/adapters/{table_adapter_test.go => table_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename go/libraries/doltcore/sqle/adapters/{table_adapter.go => table.go} (100%) rename go/libraries/doltcore/sqle/adapters/{table_adapter_test.go => table_test.go} (100%) diff --git a/go/libraries/doltcore/sqle/adapters/table_adapter.go b/go/libraries/doltcore/sqle/adapters/table.go similarity index 100% rename from go/libraries/doltcore/sqle/adapters/table_adapter.go rename to go/libraries/doltcore/sqle/adapters/table.go diff --git a/go/libraries/doltcore/sqle/adapters/table_adapter_test.go b/go/libraries/doltcore/sqle/adapters/table_test.go similarity index 100% rename from go/libraries/doltcore/sqle/adapters/table_adapter_test.go rename to go/libraries/doltcore/sqle/adapters/table_test.go From d956afb08a95348b0d629feaebd2ae3118175478 Mon Sep 17 00:00:00 2001 From: elianddb Date: Tue, 9 Dec 2025 03:30:19 -0800 Subject: [PATCH 20/22] rm privs check in dolt_purge_dropped_databases as it is already handled --- .../dolt_purge_dropped_databases.go | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_purge_dropped_databases.go b/go/libraries/doltcore/sqle/dprocedures/dolt_purge_dropped_databases.go index 63919e0941..e899868291 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_purge_dropped_databases.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_purge_dropped_databases.go @@ -27,11 +27,6 @@ func doltPurgeDroppedDatabases(ctx *sql.Context, args ...string) (sql.RowIter, e return nil, fmt.Errorf("dolt_purge_dropped_databases does not take any arguments") } - // Only allow admins to purge dropped databases - if err := checkDoltPurgeDroppedDatabasesPrivs(ctx); err != nil { - return nil, err - } - doltSession := dsess.DSessFromSess(ctx.Session) err := doltSession.Provider().PurgeDroppedDatabases(ctx) if err != nil { @@ -40,18 +35,3 @@ func doltPurgeDroppedDatabases(ctx *sql.Context, args ...string) (sql.RowIter, e return rowToIter(int64(cmdSuccess)), nil } - -// checkDoltPurgeDroppedDatabasesPrivs returns an error if the user requesting to purge dropped databases -// does not have SUPER access. Since this is a permanent and destructive operation, we restrict it to admins, -// even though the SUPER privilege has been deprecated, since there isn't another appropriate global privilege. -func checkDoltPurgeDroppedDatabasesPrivs(ctx *sql.Context) error { - privs, counter := ctx.GetPrivilegeSet() - if counter == 0 { - return fmt.Errorf("unable to check user privileges for dolt_purge_dropped_databases procedure") - } - if privs.Has(sql.PrivilegeType_Super) == false { - return sql.ErrPrivilegeCheckFailed.New(ctx.Session.Client().User) - } - - return nil -} From 904f8e281c5aca7dab7a310e89bb8281a943b328 Mon Sep 17 00:00:00 2001 From: elianddb Date: Tue, 9 Dec 2025 22:30:28 +0000 Subject: [PATCH 21/22] [ga-format-pr] Run go/utils/repofmt/format_repo.sh and go/Godeps/update.sh --- go/cmd/dolt/commands/diff.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go/cmd/dolt/commands/diff.go b/go/cmd/dolt/commands/diff.go index f739ae9d9e..76b5b1b688 100644 --- a/go/cmd/dolt/commands/diff.go +++ b/go/cmd/dolt/commands/diff.go @@ -30,8 +30,6 @@ import ( "github.com/gocraft/dbr/v2" "github.com/gocraft/dbr/v2/dialect" - eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1" - "github.com/dolthub/dolt/go/cmd/dolt/cli" "github.com/dolthub/dolt/go/cmd/dolt/errhand" "github.com/dolthub/dolt/go/libraries/doltcore/diff" @@ -44,6 +42,7 @@ import ( "github.com/dolthub/dolt/go/libraries/utils/argparser" "github.com/dolthub/dolt/go/libraries/utils/iohelp" "github.com/dolthub/dolt/go/libraries/utils/set" + eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1" ) type diffOutput int From a354d667a05d6eeee2d2be77eb502ff7d644da1a Mon Sep 17 00:00:00 2001 From: elianddb Date: Wed, 10 Dec 2025 10:16:38 -0800 Subject: [PATCH 22/22] add copyright in adapters/table.go --- go/libraries/doltcore/sqle/adapters/table.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/go/libraries/doltcore/sqle/adapters/table.go b/go/libraries/doltcore/sqle/adapters/table.go index 12c1db1dad..de5fa3c209 100644 --- a/go/libraries/doltcore/sqle/adapters/table.go +++ b/go/libraries/doltcore/sqle/adapters/table.go @@ -1,3 +1,17 @@ +// Copyright 2025 Dolthub, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package adapters import (