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] 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 }