diff --git a/go/libraries/doltcore/merge/merge_prolly_rows.go b/go/libraries/doltcore/merge/merge_prolly_rows.go index fb1f39a4ba..9e653afce5 100644 --- a/go/libraries/doltcore/merge/merge_prolly_rows.go +++ b/go/libraries/doltcore/merge/merge_prolly_rows.go @@ -133,17 +133,17 @@ func mergeProllyTableData(ctx *sql.Context, tm *TableMerger, finalSch schema.Sch if err != nil { return nil, nil, err } - leftRows := durable.ProllyMapFromIndex(lr) + leftEditor := durable.ProllyMapFromIndex(lr).Mutate() ai, err := mergeTbl.GetArtifacts(ctx) if err != nil { return nil, nil, err } - ae := durable.ProllyMapFromArtifactIndex(ai).Editor() + artEditor := durable.ProllyMapFromArtifactIndex(ai).Editor() keyless := schema.IsKeyless(tm.leftSch) - pri, err := newPrimaryMerger(leftRows, tm, valueMerger, finalSch) + pri, err := newPrimaryMerger(leftEditor, tm, valueMerger, finalSch) if err != nil { return nil, nil, err } @@ -151,18 +151,18 @@ func mergeProllyTableData(ctx *sql.Context, tm *TableMerger, finalSch schema.Sch if err != nil { return nil, nil, err } - conflicts, err := newConflictMerger(ctx, tm, ae) + conflicts, err := newConflictMerger(ctx, tm, artEditor) if err != nil { return nil, nil, err } // validator shares an artifact editor with conflict merge - uniq, err := newUniqValidator(ctx, finalSch, tm, valueMerger, ae) + uniq, err := newUniqValidator(ctx, finalSch, tm, valueMerger, artEditor) if err != nil { return nil, nil, err } - nullChk, err := newNullValidator(ctx, finalSch, tm, valueMerger, ae) + nullChk, err := newNullValidator(ctx, finalSch, tm, valueMerger, artEditor, leftEditor, sec.leftMut) if err != nil { return nil, nil, err } @@ -513,31 +513,51 @@ type nullValidator struct { // leftMap and rightMap map value tuples to |final| leftMap, rightMap val.OrdinalMapping // edits is the artifacts maps editor - edits *prolly.ArtifactsEditor - // theirRootish is the hash.Hash of the right-side - // rootish being merged + artEditor *prolly.ArtifactsEditor + // leftEdits if the left-side row editor + leftEditor *prolly.MutableMap + // secEditors are the secondary index editors + secEditors []MutableSecondaryIdx + // theirRootish is the hash.Hash of the right-side revision theirRootish hash.Hash + // ourRootish is the hash.Hash of the left-side revision + ourRootish hash.Hash } -func newNullValidator(ctx context.Context, final schema.Schema, tm *TableMerger, vm *valueMerger, edits *prolly.ArtifactsEditor) (nullValidator, error) { +func newNullValidator( + ctx context.Context, + final schema.Schema, + tm *TableMerger, + vm *valueMerger, + artEditor *prolly.ArtifactsEditor, + leftEditor *prolly.MutableMap, + secEditors []MutableSecondaryIdx, +) (nullValidator, error) { theirRootish, err := tm.rightSrc.HashOf() if err != nil { return nullValidator{}, err } + ourRootish, err := tm.rightSrc.HashOf() + if err != nil { + return nullValidator{}, err + } return nullValidator{ table: tm.name, final: final, leftMap: vm.leftMapping, rightMap: vm.rightMapping, - edits: edits, + artEditor: artEditor, + leftEditor: leftEditor, + secEditors: secEditors, theirRootish: theirRootish, + ourRootish: ourRootish, }, nil } func (nv nullValidator) validateDiff(ctx context.Context, diff tree.ThreeWayDiff) (count int, err error) { - var violations []string switch diff.Op { case tree.DiffOpRightAdd, tree.DiffOpRightModify: + var violations []string for to, from := range nv.rightMap { col := nv.final.GetNonPKCols().GetByIndex(to) if col.IsNullable() { @@ -556,7 +576,22 @@ func (nv nullValidator) validateDiff(ctx context.Context, diff tree.ThreeWayDiff } } } + // for right-side NULL violations, we insert a constraint violation and + // set |count| > 0 to signal to the caller that |diff| should not be applied + if len(violations) > 0 { + var meta prolly.ConstraintViolationMeta + if meta, err = newNotNullViolationMeta(violations, diff.Right); err != nil { + return 0, err + } + err = nv.artEditor.ReplaceConstraintViolation(ctx, diff.Key, nv.theirRootish, prolly.ArtifactTypeNullViol, meta) + if err != nil { + return 0, err + } + } + count = len(violations) + case tree.DiffOpLeftAdd, tree.DiffOpLeftModify: + var violations []string for to, from := range nv.leftMap { col := nv.final.GetNonPKCols().GetByIndex(to) if col.IsNullable() { @@ -567,32 +602,36 @@ func (nv nullValidator) validateDiff(ctx context.Context, diff tree.ThreeWayDiff // on the left side of the merge, check if it will // be populated with a default value if col.Default == "" { - // todo: we cannot record row-level conflicts originating from - // the left side of the merge, this should be a schema conflict - return 0, fmt.Errorf("table %s can't be automatically merged.\n"+ - "To merge this table, make the schema on the source and target branch equal.", nv.table) + violations = append(violations, col.Name) } } else { if diff.Left.FieldIsNull(from) { - // todo: we cannot record row-level conflicts originating from - // the left side of the merge, this should be a schema conflict - return 0, fmt.Errorf("table %s can't be automatically merged.\n"+ - "To merge this table, make the schema on the source and target branch equal.", nv.table) + violations = append(violations, col.Name) + } + } + } + // for left-side NULL violations, we insert a constraint violation and + // then must explicitly remove this row from all left-side indexes + if len(violations) > 0 { + var meta prolly.ConstraintViolationMeta + if meta, err = newNotNullViolationMeta(violations, diff.Left); err != nil { + return 0, err + } + err = nv.artEditor.ReplaceConstraintViolation(ctx, diff.Key, nv.ourRootish, prolly.ArtifactTypeNullViol, meta) + if err != nil { + return 0, err + } + if err = nv.leftEditor.Delete(ctx, diff.Key); err != nil { + return 0, err + } + for _, editor := range nv.secEditors { + if err = editor.DeleteEntry(ctx, diff.Key, diff.Left); err != nil { + return 0, err } } } } - if len(violations) > 0 { - var meta prolly.ConstraintViolationMeta - if meta, err = newNotNullViolationMeta(violations, diff.Right); err != nil { - return 0, err - } - err = nv.edits.ReplaceConstraintViolation(ctx, diff.Key, nv.theirRootish, prolly.ArtifactTypeNullViol, meta) - if err != nil { - return 0, err - } - } - return len(violations), nil + return } // conflictMerger processing primary key diffs @@ -674,9 +713,9 @@ type primaryMerger struct { finalSch schema.Schema } -func newPrimaryMerger(leftRows prolly.Map, tableMerger *TableMerger, valueMerger *valueMerger, finalSch schema.Schema) (*primaryMerger, error) { +func newPrimaryMerger(leftEditor *prolly.MutableMap, tableMerger *TableMerger, valueMerger *valueMerger, finalSch schema.Schema) (*primaryMerger, error) { return &primaryMerger{ - mut: leftRows.Mutate(), + mut: leftEditor, valueMerger: valueMerger, tableMerger: tableMerger, finalSch: finalSch, diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go b/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go index b6973e1e11..f4a46f31f6 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go @@ -49,7 +49,7 @@ var skipPrepared bool // SkipPreparedsCount is used by the "ci-check-repo CI workflow // as a reminder to consider prepareds when adding a new // enginetest suite. -const SkipPreparedsCount = 85 +const SkipPreparedsCount = 86 const skipPreparedFlag = "DOLT_SKIP_PREPARED_ENGINETESTS" @@ -160,76 +160,42 @@ func TestSingleScript(t *testing.T) { // Convenience test for debugging a single query. Unskip and set to the desired query. func TestSingleMergeScript(t *testing.T) { + t.Skip() var scripts = []MergeScriptTest{ { - Name: "adding a not-null constraint and default value to a column", + Name: "adding a non-null column with a default value to one side", AncSetUpScript: []string{ "set dolt_force_transaction_commit = on;", "create table t (pk int primary key, col1 int);", - "insert into t values (1, null), (2, null);", + "insert into t values (1, 1);", }, RightSetUpScript: []string{ - "update t set col1 = 9999 where col1 is null;", - "alter table t modify column col1 int not null default 9999;", - "insert into t values (3, 30), (4, 40);", + "alter table t add column col2 int not null default 0", + "alter table t add column col3 int;", + "update t set col2 = 1 where pk = 1;", + "insert into t values (2, 2, 2, null);", }, LeftSetUpScript: []string{ - "insert into t values (5, null), (6, null);", + "insert into t values (3, 3);", }, Assertions: []queries.ScriptTestAssertion{ { Query: "call dolt_merge('right');", }, { - Query: "select pk, col1 from t;", - Expected: []sql.Row{ - {1, 9999}, - {2, 9999}, - {3, 30}, - {4, 40}, - }, - }, - }, - }, - { - Name: "adding a not-null constraint to one side", - AncSetUpScript: []string{ - "set dolt_force_transaction_commit = on;", - "create table t (pk int primary key, col1 int);", - "insert into t values (1, null), (2, null);", - }, - RightSetUpScript: []string{ - "update t set col1 = 0 where col1 is null;", - "alter table t modify col1 int not null;", - }, - LeftSetUpScript: []string{ - "insert into t values (3, null);", - }, - Assertions: []queries.ScriptTestAssertion{ - { - Query: "call dolt_merge('right');", + Query: "select * from t;", + Expected: []sql.Row{{1, 1, 1, nil}, {2, 2, 2, nil}, {3, 3, 0, nil}}, }, { - Skip: true, - Query: "select pk, col1 from t;", - Expected: []sql.Row{ - {1, 0}, - {2, 0}, - }, - }, - { - Query: "select violation_type, pk from dolt_constraint_violations_t", - Expected: []sql.Row{ - {uint16(4), 3}, - }, + Query: "select pk, violation_type from dolt_constraint_violations_t", + Expected: []sql.Row{}, }, }, }, } - for _, test := range scripts { + enginetest.TestScript(t, newDoltHarness(t), convertMergeScriptTest(test, false)) enginetest.TestScript(t, newDoltHarness(t), convertMergeScriptTest(test, true)) - //enginetest.TestScript(t, harness, convertMergeScriptTest(test, false)) } } diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go index c9a016d747..4db1f94ac9 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go @@ -4633,6 +4633,35 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{ "create table t (pk int primary key, col1 int);", "insert into t values (1, 1);", }, + RightSetUpScript: []string{ + "alter table t add column col2 int not null default 0", + "alter table t add column col3 int;", + "insert into t values (2, 2, 2, null);", + }, + LeftSetUpScript: []string{ + "insert into t values (3, 3);", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "call dolt_merge('right');", + }, + { + Query: "select * from t;", + Expected: []sql.Row{{1, 1, 0, nil}, {2, 2, 2, nil}, {3, 3, 0, nil}}, + }, + { + Query: "select pk, violation_type from dolt_constraint_violations_t", + Expected: []sql.Row{}, + }, + }, + }, + { + Name: "adding a non-null column with a default value to one side (with update to existing row)", + AncSetUpScript: []string{ + "set dolt_force_transaction_commit = on;", + "create table t (pk int primary key, col1 int);", + "insert into t values (1, 1);", + }, RightSetUpScript: []string{ "alter table t add column col2 int not null default 0", "alter table t add column col3 int;", @@ -4648,9 +4677,13 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{ }, { Skip: true, - Query: "select * from t;", + Query: "select * from t;", // fails with row(1,1,0,NULL) Expected: []sql.Row{{1, 1, 1, nil}, {2, 2, 2, nil}, {3, 3, 0, nil}}, }, + { + Query: "select pk, violation_type from dolt_constraint_violations_t", + Expected: []sql.Row{}, + }, }, }, { @@ -4670,8 +4703,8 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{ }, Assertions: []queries.ScriptTestAssertion{ { - Skip: true, - Query: "call dolt_merge('right');", + Query: "call dolt_merge('right');", + Expected: []sql.Row{{0, 0x1}}, }, { Skip: true, @@ -4683,6 +4716,13 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{ {4, 40}, }, }, + { + Query: "select pk, violation_type from dolt_constraint_violations_t", + Expected: []sql.Row{ + {5, uint16(4)}, + {6, uint16(4)}, + }, + }, }, }, { @@ -4701,11 +4741,10 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{ }, Assertions: []queries.ScriptTestAssertion{ { - Skip: true, - Query: "call dolt_merge('right');", + Query: "call dolt_merge('right');", + Expected: []sql.Row{{0, 0x1}}, }, { - Skip: true, Query: "select pk, col1 from t;", Expected: []sql.Row{ {1, 0}, @@ -4713,7 +4752,6 @@ var ThreeWayMergeWithSchemaChangeTestScripts = []MergeScriptTest{ }, }, { - Skip: true, Query: "select violation_type, pk, violation_info from dolt_constraint_violations_t", Expected: []sql.Row{ {uint16(4), 3, types.JSONDocument{Val: merge.NullViolationMeta{Columns: []string{"col1"}}}}, diff --git a/integration-tests/bats/merge-3way-schema-changes.bats b/integration-tests/bats/merge-3way-schema-changes.bats index 818bcbaeb5..41dd936176 100644 --- a/integration-tests/bats/merge-3way-schema-changes.bats +++ b/integration-tests/bats/merge-3way-schema-changes.bats @@ -11,7 +11,7 @@ teardown() { } -@test "merge-3way-schema-changes: blocked merge can be fixed by making the schema identical" { +@test "merge-3way-schema-changes: add a NOT NULL column with default value on a branch" { dolt sql -q "create table t (pk int primary key);" dolt commit -Am "ancestor" @@ -24,20 +24,10 @@ teardown() { dolt sql -q "insert into t values (2);" dolt commit -am "left" - run dolt merge right - [ $status -ne 0 ] - [[ $output =~ "table t can't be automatically merged." ]] - - run dolt diff main right --schema -r sql - [ $status -eq 0 ] - [[ $output =~ 'ALTER TABLE `t` ADD `col1` int NOT NULL DEFAULT 0;' ]] - - dolt sql -q 'ALTER TABLE `t` ADD `col1` int NOT NULL DEFAULT 0;' - dolt commit -am "fix merge" dolt merge right - run dolt sql -r csv -q "select * from t;" - [[ $output =~ "pk,col1" ]] - [[ $output =~ "1,0" ]] - [[ $output =~ "2,0" ]] + run dolt sql -q "select * from t" -r csv + log_status_eq 0 + [[ "$output" =~ "1,0" ]] || false + [[ "$output" =~ "2,0" ]] || false } \ No newline at end of file