Refactor rebuildPrimaryIndex, rebuildSecondaryIndexes, and DiffInfo to make it more readable.

go/libraries/doltcore/merge/merge_prolly_rows.go

@@ -47,11 +47,11 @@ var ErrUnableToMergeColumnDefaultValue = errorkinds.NewKind("unable to automatic
 	"in merge: %s for table '%s'; to continue merging, first manually apply the column alteration on this branch")
 
 // mergeProllyTable merges the table specified by |tm| using the specified |mergedSch| and returns the new table
-// instance, along with merge stats and any error. If |rewriteRows| is true, then any existing rows in the
+// instance, along with merge stats and any error. If |diffInfo.RewriteRows| is true, then any existing rows in the
 // table's primary index will also be rewritten. This function merges the table's artifacts (e.g. recorded
 // conflicts), migrates any existing table data to the specified |mergedSch|, and merges table data from both
 // sides of the merge together.
-func mergeProllyTable(ctx context.Context, tm *TableMerger, mergedSch schema.Schema, diffInfo tree.ThreeWayDiffInfo) (*doltdb.Table, *MergeStats, error) {
+func mergeProllyTable(ctx context.Context, tm *TableMerger, mergedSch schema.Schema, diffInfo tree.ThreeWayDiffInfo, rebuildIndexes bool) (*doltdb.Table, *MergeStats, error) {
 	mergeTbl, err := mergeTableArtifacts(ctx, tm, tm.leftTbl)
 	if err != nil {
 		return nil, nil, err
@@ -76,11 +76,8 @@ func mergeProllyTable(ctx context.Context, tm *TableMerger, mergedSch schema.Sch
 		sqlCtx = sql.NewContext(ctx)
 	}
 
-	schemasDifferentSize := len(tm.leftSch.GetAllCols().GetColumns()) != len(mergedSch.GetAllCols().GetColumns())
-	rebuildPrimaryIndex := diffInfo.TableRewrite || schemasDifferentSize || !valueMerger.leftMapping.IsIdentityMapping()
-
 	var stats *MergeStats
-	mergeTbl, stats, err = mergeProllyTableData(sqlCtx, tm, mergedSch, mergeTbl, valueMerger, diffInfo, rebuildPrimaryIndex)
+	mergeTbl, stats, err = mergeProllyTableData(sqlCtx, tm, mergedSch, mergeTbl, valueMerger, diffInfo, rebuildIndexes)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -106,9 +103,17 @@ func mergeProllyTable(ctx context.Context, tm *TableMerger, mergedSch schema.Sch
 // as well as any secondary indexes, and also checking for unique constraints incrementally. When
 // conflicts are detected, this function attempts to resolve them automatically if possible, and
 // if not, they are recorded as conflicts in the table's artifacts. If |rebuildIndexes| is set to
-// true, then secondary indexes will be rebuilt, instead of being incrementally merged together. This
+// true, then the table indexes will be rebuilt instead of being incrementally merged together. This
 // is less efficient, but safer, especially when type changes have been applied to a table's schema.
-func mergeProllyTableData(ctx *sql.Context, tm *TableMerger, finalSch schema.Schema, mergeTbl *doltdb.Table, valueMerger *valueMerger, diffInfo tree.ThreeWayDiffInfo, rebuildPrimaryIndex bool) (*doltdb.Table, *MergeStats, error) {
+func mergeProllyTableData(ctx *sql.Context, tm *TableMerger, finalSch schema.Schema, mergeTbl *doltdb.Table, valueMerger *valueMerger, diffInfo tree.ThreeWayDiffInfo, rebuildIndexes bool) (*doltdb.Table, *MergeStats, error) {
+
+	schemasDifferentSize := len(tm.leftSch.GetAllCols().GetColumns()) != len(finalSch.GetAllCols().GetColumns())
+
+	// If columns were dropped, added, or reordered by the merge, we must rebuild the primary index,
+	// but the secondary indexes are unaffected.
+	rebuildPrimaryIndex := rebuildIndexes || schemasDifferentSize || !valueMerger.leftMapping.IsIdentityMapping()
+	rebuildSecondaryIndexes := rebuildIndexes
+
 	iter, err := threeWayDiffer(ctx, tm, valueMerger, diffInfo)
 	if err != nil {
 		return nil, nil, err
@@ -285,7 +290,7 @@ func mergeProllyTableData(ctx *sql.Context, tm *TableMerger, finalSch schema.Sch
 		return nil, nil, err
 	}
 
-	finalIdxs, err := mergeProllySecondaryIndexes(ctx, tm, leftIdxs, rightIdxs, finalSch, finalRows, conflicts.ae, diffInfo.TableRewrite)
+	finalIdxs, err := mergeProllySecondaryIndexes(ctx, tm, leftIdxs, rightIdxs, finalSch, finalRows, conflicts.ae, rebuildSecondaryIndexes)
 	if err != nil {
 		return nil, nil, err
 	}

go/libraries/doltcore/merge/merge_rows.go

@@ -126,7 +126,7 @@ func (rm *RootMerger) MergeTable(ctx *sql.Context, tblName string, opts editor.O
 	}
 
 	// Calculate a merge of the schemas, but don't apply it yet
-	mergeSch, schConflicts, diffInfo, err := SchemaMerge(ctx, tm.vrw.Format(), tm.leftSch, tm.rightSch, tm.ancSch, tblName)
+	mergeSch, schConflicts, diffInfo, rebuildIndexes, err := SchemaMerge(ctx, tm.vrw.Format(), tm.leftSch, tm.rightSch, tm.ancSch, tblName)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -148,7 +148,7 @@ func (rm *RootMerger) MergeTable(ctx *sql.Context, tblName string, opts editor.O
 
 	var tbl *doltdb.Table
 	if types.IsFormat_DOLT(tm.vrw.Format()) {
-		tbl, stats, err = mergeProllyTable(ctx, tm, mergeSch, diffInfo)
+		tbl, stats, err = mergeProllyTable(ctx, tm, mergeSch, diffInfo, rebuildIndexes)
 	} else {
 		tbl, stats, err = mergeNomsTable(ctx, tm, mergeSch, rm.vrw, opts)
 	}

go/libraries/doltcore/merge/merge_schema.go

@@ -156,7 +156,7 @@ var ErrMergeWithDifferentPks = errors.New("error: cannot merge two tables with d
 // 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
 // unexpected error encountered while merging the schemas.
-func SchemaMerge(ctx context.Context, format *storetypes.NomsBinFormat, ourSch, theirSch, ancSch schema.Schema, tblName string) (sch schema.Schema, sc SchemaConflict, diffInfo tree.ThreeWayDiffInfo, err error) {
+func SchemaMerge(ctx context.Context, format *storetypes.NomsBinFormat, ourSch, theirSch, ancSch schema.Schema, tblName string) (sch schema.Schema, sc SchemaConflict, diffInfo tree.ThreeWayDiffInfo, rebuildIndexes bool, err error) {
 	// (sch - ancSch) ∪ (mergeSch - ancSch) ∪ (sch ∩ mergeSch)
 	sc = SchemaConflict{
 		TableName: tblName,
@@ -165,38 +165,38 @@ 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{}, diffInfo, ErrMergeWithDifferentPks
+		return nil, SchemaConflict{}, diffInfo, rebuildIndexes, ErrMergeWithDifferentPks
 	}
 
 	var mergedCC *schema.ColCollection
-	mergedCC, sc.ColConflicts, diffInfo, err = mergeColumns(tblName, format, ourSch.GetAllCols(), theirSch.GetAllCols(), ancSch.GetAllCols())
+	mergedCC, sc.ColConflicts, diffInfo, rebuildIndexes, err = mergeColumns(tblName, format, ourSch.GetAllCols(), theirSch.GetAllCols(), ancSch.GetAllCols())
 	if err != nil {
-		return nil, SchemaConflict{}, diffInfo, err
+		return nil, SchemaConflict{}, diffInfo, rebuildIndexes, err
 	}
 	if len(sc.ColConflicts) > 0 {
-		return nil, sc, diffInfo, nil
+		return nil, sc, diffInfo, rebuildIndexes, nil
 	}
 
 	var mergedIdxs schema.IndexCollection
 	mergedIdxs, sc.IdxConflicts = mergeIndexes(mergedCC, ourSch, theirSch, ancSch)
 	if len(sc.IdxConflicts) > 0 {
-		return nil, sc, diffInfo, nil
+		return nil, sc, diffInfo, rebuildIndexes, nil
 	}
 
 	sch, err = schema.SchemaFromCols(mergedCC)
 	if err != nil {
-		return nil, sc, diffInfo, err
+		return nil, sc, diffInfo, rebuildIndexes, err
 	}
 
 	sch, err = mergeTableCollation(ctx, tblName, ancSch, ourSch, theirSch, sch)
 	if err != nil {
-		return nil, sc, diffInfo, err
+		return nil, sc, diffInfo, rebuildIndexes, err
 	}
 
 	// TODO: Merge conflict should have blocked any primary key ordinal changes
 	err = sch.SetPkOrdinals(ourSch.GetPkOrdinals())
 	if err != nil {
-		return nil, sc, diffInfo, err
+		return nil, sc, diffInfo, rebuildIndexes, err
 	}
 
 	_ = mergedIdxs.Iter(func(index schema.Index) (stop bool, err error) {
@@ -208,17 +208,17 @@ func SchemaMerge(ctx context.Context, format *storetypes.NomsBinFormat, ourSch,
 	var mergedChks []schema.Check
 	mergedChks, sc.ChkConflicts, err = mergeChecks(ctx, ourSch.Checks(), theirSch.Checks(), ancSch.Checks())
 	if err != nil {
-		return nil, SchemaConflict{}, diffInfo, err
+		return nil, SchemaConflict{}, diffInfo, rebuildIndexes, err
 	}
 	if len(sc.ChkConflicts) > 0 {
-		return nil, sc, diffInfo, nil
+		return nil, sc, diffInfo, rebuildIndexes, nil
 	}
 
 	// Look for invalid CHECKs
 	for _, chk := range mergedChks {
 		// CONFLICT: a CHECK now references a column that no longer exists in schema
 		if ok, err := isCheckReferenced(sch, chk); err != nil {
-			return nil, sc, diffInfo, err
+			return nil, sc, diffInfo, rebuildIndexes, err
 		} else if !ok {
 			// Append to conflicts
 			sc.ChkConflicts = append(sc.ChkConflicts, ChkConflict{
@@ -233,7 +233,7 @@ func SchemaMerge(ctx context.Context, format *storetypes.NomsBinFormat, ourSch,
 		sch.Checks().AddCheck(chk.Name(), chk.Expression(), chk.Enforced())
 	}
 
-	return sch, sc, diffInfo, nil
+	return sch, sc, diffInfo, rebuildIndexes, nil
 }
 
 // ForeignKeysMerge performs a three-way merge of (ourRoot, theirRoot, ancRoot) and using mergeRoot to validate FKs.
@@ -370,20 +370,20 @@ func checkUnmergeableNewColumns(tblName string, columnMappings columnMappings) e
 // compatible with the current stored format. The merged columns, any column conflicts, and a boolean value stating if
 // a full table rewrite is needed to align the existing table rows with the new, merged schema. If any unexpected error
 // occurs, then that error is returned and the other response fields should be ignored.
-func mergeColumns(tblName string, format *storetypes.NomsBinFormat, ourCC, theirCC, ancCC *schema.ColCollection) (*schema.ColCollection, []ColConflict, tree.ThreeWayDiffInfo, error) {
+func mergeColumns(tblName string, format *storetypes.NomsBinFormat, ourCC, theirCC, ancCC *schema.ColCollection) (*schema.ColCollection, []ColConflict, tree.ThreeWayDiffInfo, bool, error) {
 	columnMappings, err := mapColumns(ourCC, theirCC, ancCC)
 	if err != nil {
-		return nil, nil, tree.ThreeWayDiffInfo{}, err
+		return nil, nil, tree.ThreeWayDiffInfo{}, false, err
 	}
 
 	conflicts, err := checkSchemaConflicts(columnMappings)
 	if err != nil {
-		return nil, nil, tree.ThreeWayDiffInfo{}, err
+		return nil, nil, tree.ThreeWayDiffInfo{}, false, err
 	}
 
 	err = checkUnmergeableNewColumns(tblName, columnMappings)
 	if err != nil {
-		return nil, nil, tree.ThreeWayDiffInfo{}, err
+		return nil, nil, tree.ThreeWayDiffInfo{}, false, err
 	}
 
 	compatChecker := newTypeCompatabilityCheckerForStorageFormat(format)
@@ -498,16 +498,15 @@ func mergeColumns(tblName string, format *storetypes.NomsBinFormat, ourCC, their
 	// Check that there are no duplicate column names or tags in the merged column set
 	conflicts = append(conflicts, checkForColumnConflicts(mergedColumns)...)
 	if conflicts != nil {
-		return nil, conflicts, tree.ThreeWayDiffInfo{}, nil
+		return nil, conflicts, tree.ThreeWayDiffInfo{}, tableRewrite, nil
 	}
 
 	diffInfo := tree.ThreeWayDiffInfo{
-		TableRewrite:              tableRewrite,
 		LeftSchemaChange:          ourSchemaChanged,
 		RightSchemaChange:         theirSchemaChanged,
 		LeftAndRightSchemasDiffer: leftAndRightSchemasDiffer,
 	}
-	return schema.NewColCollection(mergedColumns...), nil, diffInfo, nil
+	return schema.NewColCollection(mergedColumns...), nil, diffInfo, tableRewrite, nil
 }
 
 // checkForColumnConflicts iterates over |mergedColumns|, checks for duplicate column names or column tags, and returns

go/libraries/doltcore/merge/schema_integration_test.go

@@ -635,8 +635,8 @@ func testMergeSchemasWithConflicts(t *testing.T, test mergeSchemaConflictTest) {
 
 	otherSch := getSchema(t, dEnv)
 
-	_, actConflicts, mergeInfo, err := merge.SchemaMerge(context.Background(), types.Format_Default, mainSch, otherSch, ancSch, "test")
-	assert.False(t, mergeInfo.TableRewrite)
+	_, actConflicts, _, rebuildIndexes, err := merge.SchemaMerge(context.Background(), types.Format_Default, mainSch, otherSch, ancSch, "test")
+	assert.False(t, rebuildIndexes)
 	if test.expectedErr != nil {
 		assert.True(t, errors.Is(err, test.expectedErr))
 		return

go/libraries/doltcore/sqle/dtables/schema_conflicts_table.go

@@ -197,7 +197,7 @@ func getCreateTableStatement(table string, sch schema.Schema, fks []doltdb.Forei
 
 func getSchemaConflictDescription(ctx context.Context, table string, base, ours, theirs schema.Schema) (string, error) {
 	nbf := noms.Format_Default
-	_, conflict, _, err := merge.SchemaMerge(ctx, nbf, ours, theirs, base, table)
+	_, conflict, _, _, err := merge.SchemaMerge(ctx, nbf, ours, theirs, base, table)
 	if err != nil {
 		return "", err
 	}

go/store/prolly/tree/three_way_differ.go

@@ -41,8 +41,14 @@ type ThreeWayDiffer[K ~[]byte, O Ordering[K]] struct {
 
 type resolveCb func(context.Context, val.Tuple, val.Tuple, val.Tuple) (val.Tuple, bool, error)
 
+// ThreeWayDiffInfo stores contextual data that can influence the diff.
+// If |LeftSchemaChange| is true, then the left side has a different schema from the base, and every row
+// in both Left and Base should be considered a modification, even if they have the same bytes.
+// If |RightSchemaChange| is true, then the right side has a different schema from the base, and every row
+// in both Right and Base should be considered a modification, even if they have the same bytes.
+// If |LeftAndRightSchemasDiffer| is true, then the left and right sides of the diff have a different schema,
+// so there cannot be any convergent edits, even if two rows in Left and Right have the same bytes.
 type ThreeWayDiffInfo struct {
-	TableRewrite              bool
 	LeftSchemaChange          bool
 	RightSchemaChange         bool
 	LeftAndRightSchemasDiffer bool