From 5ab5f499fcd85af0f2546decfb5d4c26c8f8bd99 Mon Sep 17 00:00:00 2001 From: Tan Yong Zhi Date: Sat, 10 Sep 2022 20:26:42 +0800 Subject: [PATCH] Address reviewer comments --- go/cmd/dolt/commands/diff.go | 49 +++++++++++------ integration-tests/bats/diff.bats | 91 ++++++++++++++++++++++++++++++-- 2 files changed, 121 insertions(+), 19 deletions(-) diff --git a/go/cmd/dolt/commands/diff.go b/go/cmd/dolt/commands/diff.go index c051e5c07d..6c9864acd2 100644 --- a/go/cmd/dolt/commands/diff.go +++ b/go/cmd/dolt/commands/diff.go @@ -558,7 +558,6 @@ func diffRows( if err != nil { return errhand.VerboseErrorFromError(err) } - defer rowWriter.Close(ctx) // can't diff if !diffable { @@ -618,9 +617,11 @@ func diffRows( } defer rowIter.Close(sqlCtx) + defer rowWriter.Close(ctx) + var modifiedColNames map[string]bool if dArgs.skinny { - modifiedColNames, err := getModifiedCols(sqlCtx, rowIter, unionSch, sch) + modifiedColNames, err = getModifiedCols(sqlCtx, rowIter, unionSch, sch) if err != nil { return errhand.BuildDError("Error running diff query:\n%s", query).AddCause(err).Build() } @@ -629,7 +630,7 @@ func diffRows( var filteredUnionSch sql.Schema for _, s := range unionSch { for colName := range modifiedColNames { - if strings.Contains(s.Name, colName) { + if s.Name == colName { filteredUnionSch = append(filteredUnionSch, s) } } @@ -643,17 +644,20 @@ func diffRows( defer rowWriter.Close(ctx) // reset the row iterator - rowIter.Close(sqlCtx) + err = rowIter.Close(sqlCtx) + if err != nil { + return errhand.BuildDError("Error closing row iterator:\n%s", query).AddCause(err).Build() + } _, rowIter, err = se.Query(sqlCtx, query) + defer rowIter.Close(sqlCtx) if sql.ErrSyntaxError.Is(err) { return errhand.BuildDError("Failed to parse diff query. Invalid where clause?\nDiff query: %s", query).AddCause(err).Build() } else if err != nil { return errhand.BuildDError("Error running diff query:\n%s", query).AddCause(err).Build() } - defer rowIter.Close(sqlCtx) } - err = writeDiffResults(sqlCtx, sch, unionSch, rowIter, rowWriter, dArgs.skinny) + err = writeDiffResults(sqlCtx, sch, unionSch, rowIter, rowWriter, modifiedColNames, dArgs.skinny) if err != nil { return errhand.BuildDError("Error running diff query:\n%s", query).AddCause(err).Build() } @@ -697,6 +701,7 @@ func writeDiffResults( targetSch sql.Schema, iter sql.RowIter, writer diff.SqlRowDiffWriter, + modifiedColNames map[string]bool, filterChangedCols bool, ) error { ds, err := newDiffSplitter(diffQuerySch, targetSch) @@ -719,15 +724,19 @@ func writeDiffResults( if filterChangedCols { var filteredOldRow, filteredNewRow rowDiff - for i, changeType := range oldRow.colDiffs { - if changeType != diff.None || targetSch[i].PrimaryKey { - filteredOldRow.row = append(filteredOldRow.row, oldRow.row[i]) - filteredOldRow.colDiffs = append(filteredOldRow.colDiffs, oldRow.colDiffs[i]) - filteredOldRow.rowDiff = oldRow.rowDiff + for i, changeType := range newRow.colDiffs { + if (changeType == diff.Added|diff.Removed) || modifiedColNames[targetSch[i].Name] { + if i < len(oldRow.row) { + filteredOldRow.row = append(filteredOldRow.row, oldRow.row[i]) + filteredOldRow.colDiffs = append(filteredOldRow.colDiffs, oldRow.colDiffs[i]) + filteredOldRow.rowDiff = oldRow.rowDiff + } - filteredNewRow.row = append(filteredNewRow.row, newRow.row[i]) - filteredNewRow.colDiffs = append(filteredNewRow.colDiffs, newRow.colDiffs[i]) - filteredNewRow.rowDiff = newRow.rowDiff + if i < len(newRow.row) { + filteredNewRow.row = append(filteredNewRow.row, newRow.row[i]) + filteredNewRow.colDiffs = append(filteredNewRow.colDiffs, newRow.colDiffs[i]) + filteredNewRow.rowDiff = newRow.rowDiff + } } } @@ -751,6 +760,10 @@ func writeDiffResults( } } +// getModifiedCols returns a set of the names of columns that are modified, as well as the name of the primary key for a particular row iterator and schema. +// In the case where rows are added or removed, all columns will be included +// unionSch refers to a joint schema between the schema before and after any schema changes pertaining to the diff, +// while diffQuerySch refers to the schema returned by the "dolt_diff" sql query. func getModifiedCols( ctx *sql.Context, iter sql.RowIter, @@ -769,11 +782,17 @@ func getModifiedCols( return modifiedColNames, err } - oldRow, _, err := ds.splitDiffResultRow(r) + oldRow, newRow, err := ds.splitDiffResultRow(r) if err != nil { return modifiedColNames, err } + for i, changeType := range newRow.colDiffs { + if changeType != diff.None || unionSch[i].PrimaryKey { + modifiedColNames[unionSch[i].Name] = true + } + } + for i, changeType := range oldRow.colDiffs { if changeType != diff.None || unionSch[i].PrimaryKey { modifiedColNames[unionSch[i].Name] = true diff --git a/integration-tests/bats/diff.bats b/integration-tests/bats/diff.bats index 9b6ba153b6..e8a4235ea5 100644 --- a/integration-tests/bats/diff.bats +++ b/integration-tests/bats/diff.bats @@ -720,16 +720,22 @@ SQL [[ "$output" = 'UPDATE `t` SET `val1`=30,`val3`=4 WHERE `pk`=1;' ]] || false } -@test "diff: skinny flag only shows row changed" { - dolt sql -q "create table t(pk int primary key, val1 int, val2 int)" +@test "diff: skinny flag only shows row changed without schema changes" { + dolt sql -q "CREATE TABLE t(pk int primary key, val1 int, val2 int)" dolt add . dolt sql -q "INSERT INTO t VALUES (1, 1, 1)" dolt commit -am "cm1" - dolt sql -q "UPDATE t SET val1=2 where pk=1" + run dolt diff --skinny --data HEAD~1 + [ $status -eq 0 ] + [[ "$output" =~ 'pk' ]] || false + [[ "$output" =~ 'val1' ]] || false + [[ "$output" =~ 'val2' ]] || false + + dolt sql -q "UPDATE t SET val1=2 WHERE pk=1" dolt commit -am "cm2" - dolt sql -q "UPDATE t SET val1=3 where pk=1" + dolt sql -q "UPDATE t SET val1=3 WHERE pk=1" dolt commit -am "cm3" run dolt diff --skinny HEAD~1 @@ -738,6 +744,83 @@ SQL [[ "$output" =~ 'val1' ]] || false } +@test "diff: skinny flag only shows row changed when both schema (column added) and data is changed (row updated)" { + dolt sql -q "create table t(pk int primary key, val1 int, val2 int)" + dolt add . + dolt sql -q "INSERT INTO t VALUES (1, 1, 1)" + dolt sql -q "INSERT INTO t VALUES (2, 2, 2)" + dolt commit -am "cm1" + + run dolt diff --skinny --data HEAD~1 + [ $status -eq 0 ] + [[ "$output" =~ 'pk' ]] || false + [[ "$output" =~ 'val1' ]] || false + [[ "$output" =~ 'val2' ]] || false + + dolt sql -q "UPDATE t SET val1=3 WHERE pk=1" + dolt sql -q "ALTER TABLE t ADD val3 int " + dolt sql -q "UPDATE t SET val3=4 WHERE pk=1" + dolt commit -am "cm2" + + run dolt diff --skinny --data HEAD~1 + [ $status -eq 0 ] + [[ "$output" =~ 'pk' ]] || false + [[ "$output" =~ 'val1' ]] || false + [[ "$output" =~ 'val3' ]] || false + [[ ! "$output" =~ 'val2' ]] || false +} + +@test "diff: skinny flag only shows row changed when both schema (column dropped) and data is changed (row updated)" { + dolt sql -q "create table t(pk int primary key, val1 int, s varchar(255))" + dolt add . + dolt sql -q "INSERT INTO t VALUES (1, 1, 'bla')" + dolt sql -q "INSERT INTO t VALUES (2, 2, 'bla2')" + dolt commit -am "cm1" + + run dolt diff --skinny --data HEAD~1 + [ $status -eq 0 ] + [[ "$output" =~ 'pk' ]] || false + [[ "$output" =~ 'val1' ]] || false + [[ "$output" =~ 's' ]] || false + + dolt sql -q "ALTER TABLE t DROP COLUMN s" + dolt sql -q "UPDATE t SET val1=3 WHERE pk=1" + dolt sql -q "UPDATE t SET val1=4 WHERE pk=2" + dolt commit -am "cm2" + + test=$(dolt diff --skinny --data HEAD~1) + echo $test + run dolt diff --skinny --data HEAD~1 + [ $status -eq 0 ] + [[ "$output" =~ 'pk' ]] || false + [[ "$output" =~ 'val1' ]] || false + [[ "$output" =~ 's' ]] || false + [[ ! "$output" =~ 'val3' ]] || false +} + +@test "diff: skinny flag only shows row changed when data is changed (row deleted)" { + dolt sql -q "create table t(pk int primary key, val1 int, val2 int)" + dolt add . + dolt sql -q "INSERT INTO t VALUES (1, 1, 1)" + dolt sql -q "INSERT INTO t VALUES (2, 2, 2)" + dolt commit -am "cm1" + + run dolt diff --skinny --data HEAD~1 + [ $status -eq 0 ] + [[ "$output" =~ 'pk' ]] || false + [[ "$output" =~ 'val1' ]] || false + [[ "$output" =~ 'val2' ]] || false + + dolt sql -q "DELETE FROM t WHERE pk=1" + dolt commit -am "cm2" + + run dolt diff --skinny --data HEAD~1 + [ $status -eq 0 ] + [[ "$output" =~ 'pk' ]] || false + [[ "$output" =~ 'val1' ]] || false + [[ "$output" =~ 'val2' ]] || false +} + @test "diff: keyless sql diffs" { dolt sql -q "create table t(pk int, val int)"