Merge pull request #7775 from dolthub/nicktobey/relaxconflicts

Allow updating conflict table even if the underlying table schema changed.

go/libraries/doltcore/merge/schema_integration_test.go

@@ -134,7 +134,6 @@ var mergeSchemaTests = []mergeSchemaTest{
 				newColTypeInfo("c9", uint64(4508), typeinfo.Int32Type, false)),
 			schema.NewIndex("c1_idx", []uint64{8201}, []uint64{8201, 3228}, nil, schema.IndexProperties{IsUserDefined: true}),
 		),
-		skip: true,
 	},
 	{
 		name: "add constraint, drop constraint, merge",
@@ -156,7 +155,6 @@ var mergeSchemaTests = []mergeSchemaTest{
 				newColTypeInfo("c3", uint64(4696), typeinfo.Int32Type, false)),
 			schema.NewIndex("c1_idx", []uint64{8201}, []uint64{8201, 3228}, nil, schema.IndexProperties{IsUserDefined: true}),
 		),
-		skip: true,
 	},
 	{
 		name: "add index, drop index, merge",
@@ -200,7 +198,6 @@ var mergeSchemaTests = []mergeSchemaTest{
 				newColTypeInfo("c33", uint64(4696), typeinfo.Int32Type, false)),
 			schema.NewIndex("c1_idx", []uint64{8201}, []uint64{8201, 3228}, nil, schema.IndexProperties{IsUserDefined: true}),
 		),
-		skip: true,
 	},
 	{
 		name: "rename indexes",

go/libraries/doltcore/sqle/dtables/conflicts_tables_prolly.go

@@ -463,23 +463,15 @@ type prollyConflictOurTableUpdater struct {
 }
 
 func newProllyConflictOurTableUpdater(ourUpdater sql.RowUpdater, versionMappings *versionMappings, baseSch, ourSch, theirSch schema.Schema) *prollyConflictOurTableUpdater {
-	// The schema columns need to be all equal in order for us to reliably build the PKs
-	schemaOK := schema.ColCollsAreEqual(baseSch.GetAllCols(), ourSch.GetAllCols()) &&
-		schema.ColCollsAreEqual(ourSch.GetAllCols(), theirSch.GetAllCols())
-
 	return &prollyConflictOurTableUpdater{
 		srcUpdater:      ourUpdater,
 		versionMappings: versionMappings,
 		pkOrdinals:      ourSch.GetPkOrdinals(),
-		schemaOK:        schemaOK,
 	}
 }
 
 // Update implements sql.RowUpdater. It translates updates on the conflict table to the source table.
 func (cu *prollyConflictOurTableUpdater) Update(ctx *sql.Context, oldRow sql.Row, newRow sql.Row) error {
-	if !cu.schemaOK {
-		return fmt.Errorf("the source table cannot be automatically updated through the conflict table since the base, our, and their schemas are not equal")
-	}
 
 	// Apply updates to columns prefixed with our_
 	// Updates to other columns are no-ops.
@@ -492,26 +484,6 @@ func (cu *prollyConflictOurTableUpdater) Update(ctx *sql.Context, oldRow sql.Row
 		ourNewRow[i] = newRow[j]
 	}
 
-	// Are we keyed?
-	if len(cu.pkOrdinals) > 0 {
-		// If so, set the PKs for the old row from either the base, ours, or theirs versions.
-		// This is necessary to allow updates to the conflict table to insert rows against the source table.
-		firstPkOrd := cu.pkOrdinals[0]
-
-		var mapping val.OrdinalMapping
-		if oldRow[cu.versionMappings.ourMapping[firstPkOrd]] != nil {
-			mapping = cu.versionMappings.ourMapping
-		} else if oldRow[cu.versionMappings.theirMapping[firstPkOrd]] != nil {
-			mapping = cu.versionMappings.theirMapping
-		} else {
-			mapping = cu.versionMappings.baseMapping
-		}
-
-		for _, ord := range cu.pkOrdinals {
-			ourOldRow[ord] = oldRow[mapping[ord]]
-		}
-	}
-
 	return cu.srcUpdater.Update(ctx, ourOldRow, ourNewRow)
 }
 

go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go

@@ -2950,12 +2950,30 @@ var Dolt1ConflictTableNameTableTests = []queries.ScriptTest{
 		},
 	},
 	{
-		Name:        "Updating our cols when our, their, and base schemas are not the equal errors",
+		Name:        "Updating our cols after schema change",
 		SetUpScript: append(createConflictsSetupScript, "ALTER TABLE t add column col2 int FIRST;"),
 		Assertions: []queries.ScriptTestAssertion{
 			{
-				Query:          "update dolt_conflicts_t set base_col1 = 9999, their_col1 = 9999;",
-				ExpectedErrStr: "the source table cannot be automatically updated through the conflict table since the base, our, and their schemas are not equal",
+				Query:    "show create table dolt_conflicts_t;",
+				Expected: []sql.Row{{"dolt_conflicts_t", "CREATE TABLE `dolt_conflicts_t` (\n  `from_root_ish` varchar(1023),\n  `base_pk` int,\n  `base_col1` int,\n  `our_pk` int NOT NULL,\n  `our_col2` int,\n  `our_col1` int,\n  `our_diff_type` varchar(1023),\n  `their_pk` int,\n  `their_col1` int,\n  `their_diff_type` varchar(1023),\n  `dolt_conflict_id` varchar(1023)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}},
+			},
+			{
+				Query: "select base_pk, base_col1, our_pk, our_col1, our_col2, their_pk, their_col1 from dolt_conflicts_t;",
+				Expected: []sql.Row{
+					{nil, nil, 1, -100, nil, 1, 100},
+					{nil, nil, 2, -200, nil, 2, 200},
+				},
+			},
+			{
+				Query:    "update dolt_conflicts_t set our_col2 = their_col1",
+				Expected: []sql.Row{{types.OkResult{RowsAffected: 2, Info: plan.UpdateInfo{Matched: 2, Updated: 2}}}},
+			},
+			{
+				Query: "select pk, col1, col2 from t;",
+				Expected: []sql.Row{
+					{1, -100, 100},
+					{2, -200, 200},
+				},
 			},
 		},
 	},

go/libraries/doltcore/sqle/enginetest/dolt_queries_schema_merge.go

@@ -17,6 +17,7 @@ package enginetest
 import (
 	"github.com/dolthub/go-mysql-server/enginetest/queries"
 	"github.com/dolthub/go-mysql-server/sql"
+	"github.com/dolthub/go-mysql-server/sql/plan"
 	"github.com/dolthub/go-mysql-server/sql/types"
 
 	"github.com/dolthub/dolt/go/libraries/doltcore/merge"
@@ -60,6 +61,17 @@ var SchemaChangeTestsForDataConflicts = []MergeScriptTest{
 					},
 				},
 			},
+			{
+				Query:    "update dolt_conflicts_t set our_col1 = their_col1, their_col2 = our_col2;",
+				Expected: []sql.Row{{types.OkResult{RowsAffected: 1, InsertID: 0, Info: plan.UpdateInfo{Matched: 1, Updated: 1, Warnings: 0}}}},
+			},
+			{
+				Query: "select * from t;",
+				Expected: []sql.Row{
+					{1, -100, "100", "11"},
+					{2, 20, "200", "22"},
+				},
+			},
 		},
 	},
 }
@@ -551,6 +563,63 @@ var SchemaChangeTestsBasicCases = []MergeScriptTest{
 			},
 		},
 	},
+	{
+		// Currently skipped bc of https://github.com/dolthub/dolt/issues/7767
+		Name: "ambiguous choice of ancestor column",
+		AncSetUpScript: []string{
+			"CREATE table t (pk int primary key, col1 int, col2 int);",
+			"INSERT into t values (1, 10, 100), (2, 20, 200);",
+		},
+		RightSetUpScript: []string{
+			"alter table t drop column col2;",
+			"insert into t values (3, 30), (4, 40);",
+		},
+		LeftSetUpScript: []string{
+			"alter table t drop column col1;",
+			"alter table t rename column col2 to col1;",
+			"insert into t values (5, 50), (6, 60);",
+		},
+		Assertions: []queries.ScriptTestAssertion{
+			{
+				Skip:           true,
+				Query:          "call dolt_merge('right');",
+				ExpectedErrStr: "Merge conflict detected, @autocommit transaction rolled back. @autocommit must be disabled so that merge conflicts can be resolved using the dolt_conflicts and dolt_schema_conflicts tables before manually committing the transaction. Alternatively, to commit transactions with merge conflicts, set @@dolt_allow_commit_conflicts = 1",
+			},
+		},
+	},
+	{
+		// One branch makes a new column, deletes the old one, and renames.
+		// The other just has a data change.
+		Name: "creating new column to replace ancestor column",
+		AncSetUpScript: []string{
+			"CREATE table t (pk int primary key, col1 int);",
+			"INSERT into t values (1, 10), (2, 20);",
+		},
+		RightSetUpScript: []string{
+			"insert into t values (3, 30), (4, 40);",
+		},
+		LeftSetUpScript: []string{
+			"alter table t add column col2 int",
+			"update t set col2 = 10*col1",
+			"alter table t drop column col1;",
+			"alter table t rename column col2 to col1;",
+			"insert into t values (5, 50), (6, 60);",
+		},
+		Assertions: []queries.ScriptTestAssertion{
+			{
+				Query:    "call dolt_merge('right');",
+				Expected: []sql.Row{{doltCommit, 0, 0, "merge successful"}},
+			},
+			{
+				Query: "select * from t;",
+				Expected: []sql.Row{
+					{1, 100}, {2, 200},
+					{3, 30}, {4, 40},
+					{5, 50}, {6, 60},
+				},
+			},
+		},
+	},
 }
 
 var SchemaChangeTestsCollations = []MergeScriptTest{
@@ -2469,3 +2538,38 @@ var SchemaChangeTestsForJsonConflicts = []MergeScriptTest{
 		},
 	},
 }
+
+// These tests are not run because they cause panics during set-up.
+// Each one is labeled with a GitHub issue.
+var DisabledSchemaChangeTests = []MergeScriptTest{
+	{
+		Name: "panic regression test https://github.com/dolthub/dolt/issues/7762",
+		AncSetUpScript: []string{
+			"CREATE table t (pk int primary key, col1 int, col2 int);",
+			"INSERT into t values (1, 10, 100), (2, 20, 200);",
+		},
+		RightSetUpScript: []string{
+			"alter table t drop column col2;",
+			"insert into t values (3, 30), (4, 40);",
+		},
+		LeftSetUpScript: []string{
+			"alter table t drop column col1;",
+			"alter table t rename column col2 to col1;",
+			"insert into t values (5, 50), (6, 60);",
+		},
+		Assertions: []queries.ScriptTestAssertion{
+			{
+				Query:    "call dolt_merge('right');",
+				Expected: []sql.Row{{doltCommit, 0, 0, "merge successful"}},
+			},
+			{
+				Query: "select * from t;",
+				Expected: []sql.Row{
+					{1, 10}, {2, 20},
+					{3, 30}, {4, 40},
+					{5, 50}, {6, 60},
+				},
+			},
+		},
+	},
+}

go/libraries/doltcore/sqle/writer/prolly_index_writer.go

@@ -114,7 +114,7 @@ func (m prollyIndexWriter) keyFromRow(ctx context.Context, sqlRow sql.Row) (val.
 			return nil, err
 		}
 	}
-	return m.keyBld.Build(sharePool), nil
+	return m.keyBld.BuildPermissive(sharePool), nil
 }
 
 func (m prollyIndexWriter) ValidateKeyViolations(ctx context.Context, sqlRow sql.Row) error {
@@ -165,10 +165,14 @@ func (m prollyIndexWriter) Update(ctx context.Context, oldRow sql.Row, newRow sq
 		return err
 	}
 
-	// todo(andy): we can skip building, deleting |oldKey|
-	//  if we know the key fields are unchanged
-	if err := m.mut.Delete(ctx, oldKey); err != nil {
-		return err
+	// If the old row is empty, there is nothing to delete.
+	// This can happen when updating a row in a conflict table if the row did not exist on one branch.
+	if oldKey.Count() != 0 {
+		// todo(andy): we can skip building, deleting |oldKey|
+		//  if we know the key fields are unchanged
+		if err := m.mut.Delete(ctx, oldKey); err != nil {
+			return err
+		}
 	}
 
 	newKey, err := m.keyFromRow(ctx, newRow)