dolt/dolthub#1430: Add --filter option for dolt diff

[codeaucafe]

Summary

TL;DR

There was no action on the original #3499 for issue #1430; the PR was closed ~3 years ago. This PR fixes the open PR comments and updates the implementation details a bit for the RowWriting of filtered rows

Description

Adds --filter option to dolt diff for filtering by change type (added, modified, renamed, dropped, or removed as alias for dropped); please note I added renamed and dropped alias to this filter list. Completes PR #3499 by fixing the issues that blocked it from merging, plus fixes an additional bug where empty table headers were
printed when all rows were filtered out.

Users reviewing large diffs often need to focus on specific change types - deletes may need extra scrutiny while inserts are routine. With diffs spanning thousands of rows across multiple tables, grep isn't enough since updates show
both additions and deletions.

Implementation

Core changes:

• Added diffTypeFilter struct with validation methods in diff.go
• Added --filter argument in arg_parser_helpers.go
• Table filtering uses delta.SchemaChange and delta.IsRename() for modification detection
• Row filtering checks diff.ChangeType in writeDiffResults()
• Fixed control flow - uses continue instead of return nil so all tables get processed

Empty header bug fix:

• Implemented lazyRowWriter that delays BeginTable() until first row write
• Added shouldUseLazyHeader() helper to determine when lazy initialization is needed
• Only applies to filtered data-only diffs (no schema changes or renames)
• Prevents empty table headers from printing when all rows are filtered out

Tests:

• go/cmd/dolt/commands/diff_filter_test.go - comprehensive unit tests:
  • TestShouldUseLazyHeader - validates lazy header logic (8 test cases)
  • TestLazyRowWriter_* - tests lazy initialization behavior (4 test functions)
  • Mock implementations of diffWriter and SqlRowDiffWriter interfaces
• BATS integration tests for all filter types with tables, rows, and schema changes
• Edge cases: invalid values, typos, case sensitivity

What Was Broken in PR #3499

The original PR had three blocking issues from code review comments that I fixed here:

1. No row-level filtering - only filtered tables, not individual rows

   • Fixed by adding filtering in writeDiffResults()

2. Incomplete modification detection - didn't check for schema changes properly

   • Fixed by using delta.SchemaChange || delta.IsRename() as requested

3. Loop exits early - return nil prevented processing remaining tables

   • Fixed by using continue instead

Additional Bug (bug from my initial filter changes; not existing prior) Fixed

Empty table headers when filtering:
When using --filter with data-only changes, if all rows were filtered out, dolt would still print the table diff header with no content:

# Before fix:
$ dolt diff HEAD~1 --filter=modified  # (table has only added rows)
diff --dolt a/t b/t
--- a/t
+++ b/t
(empty - no rows printed)

# After fix:
$ dolt diff HEAD~1 --filter=modified
(completely empty - no output at all)

This occurred because BeginTable() was called before row filtering. The lazyRowWriter pattern delays header output until we confirm at least one row will be written.

Usage/Examples

dolt diff --filter=added      # new tables/rows
dolt diff --filter=modified   # schema changes, row updates
dolt diff --filter=renamed    # renamed tables
dolt diff --filter=dropped    # dropped tables, deleted rows
dolt diff --filter=removed    # alias for dropped
dolt diff HEAD~1 --filter=dropped -r sql

Fixes #1430

[codeaucafe]

I believe the two failing bats tests are unrelated to my changes. I believe a perms issue on my part?

also the one test failing in Test Go/Go tests (ubuntu-22.04) is for TestMergeConcurrency which fails for ubuntu with race detected error but passes for mac and windows. Is this a known flaky test? If its not I can look into this later when I get back from short vacation this week

[elianddb]

@codeaucafe A couple of comments mainly on integrating your solution with our existing infrastructure under table_deltas.go. Let me know if you have any questions.

[elianddb]

Should follow the NameParam naming scheme

[elianddb]

Keep the same post-tense wording, ie FilterAdded

[elianddb]

Use the IsValid func here so we only have to update one place.

[elianddb]

You can use the const that should be added to apr above here for these magic values.

[elianddb]

this relates to magic apr vals

[elianddb]

Use the TableDeltaSummary object provided. This may require rewriting your impl for the filter struct above, but it keeps the logic in one place. Specifically, there's a DiffType field you can make use of.

Secondly, I think this enables you to keep track of enabled filters in a map, ie str -> bool. This also keeps the diff type strings synchronized in the future. One func can handle the checks, instead of multiple include helper funcs. Of course, impl const vars to avoid the magic vals. You should define the consts in table_deltas.go since it already defines its own names for DiffType.

[elianddb]

This information should be in the delta object already, aka TableDeltaSummary. Also, you could alternatively store the beginTable since that's your main priority.

type lazyRowWriter struct {
    realWriter  diff.SqlRowDiffWriter
    beginTable  func() error
    initialized bool
}

func (l *lazyRowWriter) WriteRow(ctx *sql.Context, row sql.Row, diffType diff.ChangeType, colDiffTypes []diff.ChangeType) error {
    if !l.initialized {
        if err := l.beginTable(); err != nil {
            return err
        }
        l.initialized = true
    }
    return l.realWriter.WriteRow(ctx, row, diffType, colDiffTypes)
}

[elianddb]

Instead of providing two different branchs to initialize rowWriter differently, wrap the original with your wrapper:

rowWriter, err := dw.RowWriter(fromTableInfo, toTableInfo, tableSummary, unionSch)
if err != nil {
    return errhand.VerboseErrorFromError(err)
}

if shouldUseLazyHeader(dArgs, tableSummary) {
    rowWriter = &lazyRowWriter{
        rowWriter,
        func() error {
            return dw.BeginTable(
                tableSummary.FromTableName.Name,
                tableSummary.ToTableName.Name,
                tableSummary.IsAdd(),
                tableSummary.IsDrop(),
            )
        },
    }
}

[elianddb]

This goes back to using the existing summary DiffType field and adding consts for the strings. Make sure to update the helper funcs. You could check the map for the necessary fields and flip using !.

[codeaucafe]

Thanks @elianddb. Sorry for the delay. I have not been able to take a look at your comments yet due to having to work very late last couple days and helping with some family stuff. I'll be sure to review your comments and ask you any questions I have when I get time to work on addressing your comments this weekend.

Thank you!

Cheers,
David

[codeaucafe]

@elianddb I updated the code based on your comments. Let me know if I misunderstood something and implemented it wrong.

I believe the failing bats test are unrelated and failing for same reasons those other unrelated bats tests failed for

Thank you!

[codeaucafe]

FYI, used the new diff type constants added here so we aren't used string literals here

[codeaucafe]

how is this looking @elianddb?

Also, instead of having initialized just added nil check on onFirstWrite

[codeaucafe]

Updated to include renamed filter, as well as using removed for alias for dropped DiffTypeDropped

[codeaucafe]

removed filter value alias for DiffTypeDropped. Figure for filtering removed is better terminology but shouldn't be specific diff type value.