From 2dd26b6b28a57b381f70454137019d5164da14f7 Mon Sep 17 00:00:00 2001 From: Nick Tobey Date: Tue, 9 Apr 2024 13:03:06 -0700 Subject: [PATCH 1/3] Improve error messaging when encountering a primary key schema change during a merge. --- go/libraries/doltcore/merge/merge_schema.go | 11 ++++-- .../doltcore/merge/schema_integration_test.go | 6 +-- .../doltcore/sqle/enginetest/dolt_queries.go | 2 +- .../sqle/enginetest/dolt_queries_merge.go | 2 +- integration-tests/bats/cherry-pick.bats | 2 +- .../bats/primary-key-changes.bats | 38 +++++++++++++++++-- integration-tests/bats/sql-cherry-pick.bats | 2 +- 7 files changed, 48 insertions(+), 15 deletions(-) diff --git a/go/libraries/doltcore/merge/merge_schema.go b/go/libraries/doltcore/merge/merge_schema.go index 442fa0c11b..ed124a3aa4 100644 --- a/go/libraries/doltcore/merge/merge_schema.go +++ b/go/libraries/doltcore/merge/merge_schema.go @@ -16,7 +16,6 @@ package merge import ( "context" - "errors" "fmt" "sort" "strings" @@ -159,7 +158,8 @@ func (c ChkConflict) String() string { return "" } -var ErrMergeWithDifferentPks = errors.New("error: cannot merge two tables with different primary keys") +var ErrMergeWithDifferentPks = errorkinds.NewKind("error: cannot merge because table %s has different primary keys") +var ErrMergeWithDifferentPksFromAncestor = errorkinds.NewKind("error: cannot merge because table %s has different primary keys in its common ancestor") // SchemaMerge performs a three-way merge of |ourSch|, |theirSch|, and |ancSch|, and returns: the merged schema, // any schema conflicts identified, whether moving to the new schema requires a full table rewrite, and any @@ -172,8 +172,11 @@ func SchemaMerge(ctx context.Context, format *storetypes.NomsBinFormat, ourSch, // TODO: We'll remove this once it's possible to get diff and merge on different primary key sets // TODO: decide how to merge different orders of PKS - if !schema.ArePrimaryKeySetsDiffable(format, ourSch, theirSch) || !schema.ArePrimaryKeySetsDiffable(format, ourSch, ancSch) { - return nil, SchemaConflict{}, mergeInfo, diffInfo, ErrMergeWithDifferentPks + if !schema.ArePrimaryKeySetsDiffable(format, ourSch, theirSch) { + return nil, SchemaConflict{}, mergeInfo, diffInfo, ErrMergeWithDifferentPks.New(tblName) + } + if !schema.ArePrimaryKeySetsDiffable(format, ourSch, ancSch) { + return nil, SchemaConflict{}, mergeInfo, diffInfo, ErrMergeWithDifferentPksFromAncestor.New(tblName) } var mergedCC *schema.ColCollection diff --git a/go/libraries/doltcore/merge/schema_integration_test.go b/go/libraries/doltcore/merge/schema_integration_test.go index af734f34a7..f797bebe29 100644 --- a/go/libraries/doltcore/merge/schema_integration_test.go +++ b/go/libraries/doltcore/merge/schema_integration_test.go @@ -16,7 +16,6 @@ package merge_test import ( "context" - "errors" "testing" "github.com/dolthub/go-mysql-server/sql" @@ -460,7 +459,7 @@ var mergeSchemaConflictTests = []mergeSchemaConflictTest{ {commands.CommitCmd{}, []string{"-m", "modified branch other"}}, {commands.CheckoutCmd{}, []string{env.DefaultInitBranch}}, }, - expectedErr: merge.ErrMergeWithDifferentPks, + expectedErr: merge.ErrMergeWithDifferentPks.New("test"), }, } @@ -638,7 +637,8 @@ func testMergeSchemasWithConflicts(t *testing.T, test mergeSchemaConflictTest) { _, actConflicts, mergeInfo, _, err := merge.SchemaMerge(context.Background(), types.Format_Default, mainSch, otherSch, ancSch, "test") assert.False(t, mergeInfo.InvalidateSecondaryIndexes) if test.expectedErr != nil { - assert.True(t, errors.Is(err, test.expectedErr)) + // We don't use errors.Is here because errors generated by `Kind.New` compare stack traces in their `Is` implementation. + assert.Equal(t, err.Error(), test.expectedErr.Error(), "Expected error '%s', instead got '%s'", test.expectedErr.Error(), err.Error()) return } diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go index 56a9f2031c..2ff577ff46 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go @@ -5447,7 +5447,7 @@ var DoltCherryPickTests = []queries.ScriptTest{ Assertions: []queries.ScriptTestAssertion{ { Query: "CALL Dolt_Cherry_Pick(@commit1);", - ExpectedErrStr: "error: cannot merge two tables with different primary keys", + ExpectedErrStr: "error: cannot merge because table t has different primary keys", }, }, }, diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go index 3e2d450f01..09a2498431 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go @@ -2105,7 +2105,7 @@ var MergeScripts = []queries.ScriptTest{ Assertions: []queries.ScriptTestAssertion{ { Query: "CALL DOLT_MERGE('right');", - ExpectedErrStr: "error: cannot merge two tables with different primary keys", + ExpectedErrStr: "error: cannot merge because table t has different primary keys", }, }, }, diff --git a/integration-tests/bats/cherry-pick.bats b/integration-tests/bats/cherry-pick.bats index d01daa19ef..23f7dea86f 100644 --- a/integration-tests/bats/cherry-pick.bats +++ b/integration-tests/bats/cherry-pick.bats @@ -609,5 +609,5 @@ teardown() { dolt checkout main run dolt cherry-pick branch1 [ $status -eq 1 ] - [[ $output =~ "error: cannot merge two tables with different primary keys" ]] || false + [[ $output =~ "error: cannot merge because table test has different primary keys" ]] || false } diff --git a/integration-tests/bats/primary-key-changes.bats b/integration-tests/bats/primary-key-changes.bats index d15160663d..1acce9be5d 100644 --- a/integration-tests/bats/primary-key-changes.bats +++ b/integration-tests/bats/primary-key-changes.bats @@ -261,7 +261,7 @@ teardown() { run dolt merge test -m "merge other" [ "$status" -eq 1 ] - [[ "$output" =~ 'error: cannot merge two tables with different primary keys' ]] || false + [[ "$output" =~ 'error: cannot merge because table t has different primary keys' ]] || false } @test "primary-key-changes: merge on branch with primary key added throws an error" { @@ -288,7 +288,37 @@ teardown() { run dolt merge test -m "merge other" [ "$status" -eq 1 ] - [[ "$output" =~ 'error: cannot merge two tables with different primary keys' ]] || false + [[ "$output" =~ 'error: cannot merge because table t has different primary keys' ]] || false +} + +@test "primary-key-changes: merge on branches with different primary key from ancestor throws an error" { + dolt sql -q "create table t(pk int, val1 int, val2 int)" + dolt add . + dolt sql -q "INSERT INTO t values (1,1,1)" + dolt commit -am "cm1" + dolt checkout -b test + + dolt sql -q "ALTER TABLE t add PRIMARY key (pk)" + dolt add . + + run dolt status + [[ "$output" =~ 'Changes to be committed' ]] || false + [[ "$output" =~ ([[:space:]]*modified:[[:space:]]*t) ]] || false + ! [[ "$output" =~ 'deleted' ]] || false + ! [[ "$output" =~ 'new table' ]] || false + + dolt commit -m "cm2" + dolt checkout main + + dolt sql -q "ALTER TABLE t add PRIMARY key (pk)" + dolt sql -q "INSERT INTO t values (2,2,2)" + dolt commit -am "cm3" + + run dolt merge test -m "merge other" + [ "$status" -eq 1 ] + "error: cannot merge because table %s has different primary keys in its common ancestor") + + [[ "$output" =~ 'error: cannot merge because table t has different primary keys' ]] || false } @test "primary-key-changes: diff on primary key schema change shows schema level diff but does not show row level diff" { @@ -527,11 +557,11 @@ SQL run dolt merge test -m "merge other" [ "$status" -eq 1 ] - [[ "$output" =~ 'error: cannot merge two tables with different primary keys' ]] || false + [[ "$output" =~ 'error: cannot merge because table t has different primary keys' ]] || false run dolt sql -q "call dolt_merge('test')" [ "$status" -eq 1 ] - [[ "$output" =~ 'error: cannot merge two tables with different primary keys' ]] || false + [[ "$output" =~ 'error: cannot merge because table t has different primary keys' ]] || false skip "Dolt doesn't correctly store primary key order if it doesn't match the column order" } diff --git a/integration-tests/bats/sql-cherry-pick.bats b/integration-tests/bats/sql-cherry-pick.bats index 81a8364200..80e33b9cf6 100644 --- a/integration-tests/bats/sql-cherry-pick.bats +++ b/integration-tests/bats/sql-cherry-pick.bats @@ -506,5 +506,5 @@ SQL dolt checkout main run dolt sql -q "CALL DOLT_CHERRY_PICK('branch1')" [ $status -eq 1 ] - [[ $output =~ "error: cannot merge two tables with different primary keys" ]] || false + [[ $output =~ "error: cannot merge because table t has different primary keys" ]] || false } From 5dc2086bce9f49f1dd9aa4045fc580d52a63ff0a Mon Sep 17 00:00:00 2001 From: Nick Tobey Date: Tue, 9 Apr 2024 13:23:13 -0700 Subject: [PATCH 2/3] Fix type in primary-key-changes.bats --- integration-tests/bats/primary-key-changes.bats | 1 - 1 file changed, 1 deletion(-) diff --git a/integration-tests/bats/primary-key-changes.bats b/integration-tests/bats/primary-key-changes.bats index 1acce9be5d..49536f01be 100644 --- a/integration-tests/bats/primary-key-changes.bats +++ b/integration-tests/bats/primary-key-changes.bats @@ -316,7 +316,6 @@ teardown() { run dolt merge test -m "merge other" [ "$status" -eq 1 ] - "error: cannot merge because table %s has different primary keys in its common ancestor") [[ "$output" =~ 'error: cannot merge because table t has different primary keys' ]] || false } From bfe2e3f58b77acaefd4852238e7e7ffcdb53256e Mon Sep 17 00:00:00 2001 From: Nick Tobey Date: Tue, 9 Apr 2024 14:51:39 -0700 Subject: [PATCH 3/3] Update bats test. --- integration-tests/bats/sql-cherry-pick.bats | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/bats/sql-cherry-pick.bats b/integration-tests/bats/sql-cherry-pick.bats index 80e33b9cf6..5942736b80 100644 --- a/integration-tests/bats/sql-cherry-pick.bats +++ b/integration-tests/bats/sql-cherry-pick.bats @@ -506,5 +506,5 @@ SQL dolt checkout main run dolt sql -q "CALL DOLT_CHERRY_PICK('branch1')" [ $status -eq 1 ] - [[ $output =~ "error: cannot merge because table t has different primary keys" ]] || false + [[ $output =~ "error: cannot merge because table test has different primary keys" ]] || false }