go/doltcore/merge: register not null constraint violations for left-side NULLs

2026-02-18 00:56:03 -06:00 · 2023-05-10 15:58:57 -07:00
parent 29ed451728
commit 1186d6de80
4 changed files with 136 additions and 103 deletions
--- 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,
--- 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))
 	}
 }

--- 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"}}}},
--- 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
 }