Skip to content

Commit

Permalink
Post-merge fixup.
Browse files Browse the repository at this point in the history
  • Loading branch information
nicktobey committed Nov 27, 2023
1 parent ececb88 commit abcaea2
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 44 deletions.
8 changes: 4 additions & 4 deletions go/libraries/doltcore/merge/merge_prolly_rows.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ var ErrUnableToMergeColumnDefaultValue = errorkinds.NewKind("unable to automatic
// 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, mergeInfo MergeInfo, diffInfo tree.ThreeWayDiffInfo) (*doltdb.Table, *MergeStats, error) {
mergeTbl, err := mergeTableArtifacts(ctx, tm, tm.leftTbl)
if err != nil {
return nil, nil, err
Expand Down Expand Up @@ -85,7 +85,7 @@ func mergeProllyTable(ctx context.Context, tm *TableMerger, mergedSch schema.Sch
}

var stats *MergeStats
mergeTbl, stats, err = mergeProllyTableData(sqlCtx, tm, mergedSch, mergeTbl, valueMerger, mergeInfo)
mergeTbl, stats, err = mergeProllyTableData(sqlCtx, tm, mergedSch, mergeTbl, valueMerger, mergeInfo, diffInfo)
if err != nil {
return nil, nil, err
}
Expand All @@ -111,8 +111,8 @@ 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.
func mergeProllyTableData(ctx *sql.Context, tm *TableMerger, finalSch schema.Schema, mergeTbl *doltdb.Table, valueMerger *valueMerger, mergeInfo MergeInfo) (*doltdb.Table, *MergeStats, error) {
iter, err := threeWayDiffer(ctx, tm, valueMerger)
func mergeProllyTableData(ctx *sql.Context, tm *TableMerger, finalSch schema.Schema, mergeTbl *doltdb.Table, valueMerger *valueMerger, mergeInfo MergeInfo, diffInfo tree.ThreeWayDiffInfo) (*doltdb.Table, *MergeStats, error) {
iter, err := threeWayDiffer(ctx, tm, valueMerger, diffInfo)
if err != nil {
return nil, nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions go/libraries/doltcore/merge/merge_rows.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, mergeInfo, err := SchemaMerge(ctx, tm.vrw.Format(), tm.leftSch, tm.rightSch, tm.ancSch, tblName)
mergeSch, schConflicts, mergeInfo, diffInfo, err := SchemaMerge(ctx, tm.vrw.Format(), tm.leftSch, tm.rightSch, tm.ancSch, tblName)
if err != nil {
return nil, nil, err
}
Expand All @@ -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, mergeInfo)
tbl, stats, err = mergeProllyTable(ctx, tm, mergeSch, mergeInfo, diffInfo)
} else {
tbl, stats, err = mergeNomsTable(ctx, tm, mergeSch, rm.vrw, opts)
}
Expand Down
75 changes: 38 additions & 37 deletions go/libraries/doltcore/merge/merge_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, mergeInfo MergeInfo, err error) {
func SchemaMerge(ctx context.Context, format *storetypes.NomsBinFormat, ourSch, theirSch, ancSch schema.Schema, tblName string) (sch schema.Schema, sc SchemaConflict, mergeInfo MergeInfo, diffInfo tree.ThreeWayDiffInfo, err error) {
// (sch - ancSch) ∪ (mergeSch - ancSch) ∪ (sch ∩ mergeSch)
sc = SchemaConflict{
TableName: tblName,
Expand All @@ -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{}, mergeInfo, ErrMergeWithDifferentPks
return nil, SchemaConflict{}, mergeInfo, diffInfo, ErrMergeWithDifferentPks
}

var mergedCC *schema.ColCollection
mergedCC, sc.ColConflicts, mergeInfo, err = mergeColumns(tblName, format, ourSch.GetAllCols(), theirSch.GetAllCols(), ancSch.GetAllCols())
mergedCC, sc.ColConflicts, mergeInfo, diffInfo, err = mergeColumns(tblName, format, ourSch.GetAllCols(), theirSch.GetAllCols(), ancSch.GetAllCols())
if err != nil {
return nil, SchemaConflict{}, mergeInfo, err
return nil, SchemaConflict{}, mergeInfo, diffInfo, err
}
if len(sc.ColConflicts) > 0 {
return nil, sc, mergeInfo, nil
return nil, sc, mergeInfo, diffInfo, nil
}

var mergedIdxs schema.IndexCollection
mergedIdxs, sc.IdxConflicts = mergeIndexes(mergedCC, ourSch, theirSch, ancSch)
if len(sc.IdxConflicts) > 0 {
return nil, sc, mergeInfo, nil
return nil, sc, mergeInfo, diffInfo, nil
}

sch, err = schema.SchemaFromCols(mergedCC)
if err != nil {
return nil, sc, mergeInfo, err
return nil, sc, mergeInfo, diffInfo, err
}

sch, err = mergeTableCollation(ctx, tblName, ancSch, ourSch, theirSch, sch)
if err != nil {
return nil, sc, mergeInfo, err
return nil, sc, mergeInfo, diffInfo, err
}

// TODO: Merge conflict should have blocked any primary key ordinal changes
err = sch.SetPkOrdinals(ourSch.GetPkOrdinals())
if err != nil {
return nil, sc, mergeInfo, err
return nil, sc, mergeInfo, diffInfo, err
}

_ = mergedIdxs.Iter(func(index schema.Index) (stop bool, err error) {
Expand All @@ -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{}, mergeInfo, err
return nil, SchemaConflict{}, mergeInfo, diffInfo, err
}
if len(sc.ChkConflicts) > 0 {
return nil, sc, mergeInfo, nil
return nil, sc, mergeInfo, diffInfo, 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, mergeInfo, err
return nil, sc, mergeInfo, diffInfo, err
} else if !ok {
// Append to conflicts
sc.ChkConflicts = append(sc.ChkConflicts, ChkConflict{
Expand All @@ -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, mergeInfo, nil
return sch, sc, mergeInfo, diffInfo, nil
}

// ForeignKeysMerge performs a three-way merge of (ourRoot, theirRoot, ancRoot) and using mergeRoot to validate FKs.
Expand Down Expand Up @@ -376,21 +376,22 @@ type MergeInfo struct {
// 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, MergeInfo, error) {
func mergeColumns(tblName string, format *storetypes.NomsBinFormat, ourCC, theirCC, ancCC *schema.ColCollection) (*schema.ColCollection, []ColConflict, MergeInfo, tree.ThreeWayDiffInfo, error) {
mergeInfo := MergeInfo{}
diffInfo := tree.ThreeWayDiffInfo{}
columnMappings, err := mapColumns(ourCC, theirCC, ancCC)
if err != nil {
return nil, nil, mergeInfo, err
return nil, nil, mergeInfo, diffInfo, err
}

conflicts, err := checkSchemaConflicts(columnMappings)
if err != nil {
return nil, nil, mergeInfo, err
return nil, nil, mergeInfo, diffInfo, err
}

err = checkUnmergeableNewColumns(tblName, columnMappings)
if err != nil {
return nil, nil, mergeInfo, err
return nil, nil, mergeInfo, diffInfo, err
}

compatChecker := newTypeCompatabilityCheckerForStorageFormat(format)
Expand All @@ -409,47 +410,47 @@ func mergeColumns(tblName string, format *storetypes.NomsBinFormat, ourCC, their
// if an ancestor does not exist, and the column exists only on one side, use that side
// (if an ancestor DOES exist, this means the column was deleted, so it's a no-op)
mergeInfo.LeftNeedsRewrite = true
theirSchemaChanged = true
leftAndRightSchemasDiffer = true
diffInfo.RightSchemaChange = true
diffInfo.LeftAndRightSchemasDiffer = true
mergedColumns = append(mergedColumns, *theirs)
case anc == nil && ours != nil && theirs == nil:
// if an ancestor does not exist, and the column exists only on one side, use that side
// (if an ancestor DOES exist, this means the column was deleted, so it's a no-op)
mergeInfo.RightNeedsRewrite = true
ourSchemaChanged = true
leftAndRightSchemasDiffer = true
diffInfo.LeftSchemaChange = true
diffInfo.LeftAndRightSchemasDiffer = true
mergedColumns = append(mergedColumns, *ours)
case anc != nil && ours == nil && theirs != nil:
// column was deleted on our side
mergeInfo.RightNeedsRewrite = true
ourSchemaChanged = true
leftAndRightSchemasDiffer = true
diffInfo.LeftSchemaChange = true
diffInfo.LeftAndRightSchemasDiffer = true
case anc != nil && ours != nil && theirs == nil:
// column was deleted on their side
mergeInfo.LeftNeedsRewrite = true
theirSchemaChanged = true
leftAndRightSchemasDiffer = true
diffInfo.RightSchemaChange = true
diffInfo.LeftAndRightSchemasDiffer = true
case ours == nil && theirs == nil:
// if the column is deleted on both sides... just let it fall out
ourSchemaChanged = true
theirSchemaChanged = true
diffInfo.LeftSchemaChange = true
diffInfo.RightSchemaChange = true
case ours != nil && theirs != nil:
// otherwise, we have two valid columns and we need to figure out which one to use
if anc != nil {
oursChanged := !anc.Equals(*ours)
ourSchemaChanged = ourSchemaChanged || oursChanged
diffInfo.LeftSchemaChange = diffInfo.LeftSchemaChange || oursChanged
theirsChanged := !anc.Equals(*theirs)
theirSchemaChanged = theirSchemaChanged || theirsChanged
diffInfo.RightSchemaChange = diffInfo.RightSchemaChange || theirsChanged
if oursChanged && theirsChanged {
// If both columns changed in the same way, the modifications converge, so accept the column.
// If not, don't report a conflict, since this case is already handled in checkForColumnConflicts.
if ours.Equals(*theirs) {
mergedColumns = append(mergedColumns, *theirs)
} else {
leftAndRightSchemasDiffer = true
diffInfo.LeftAndRightSchemasDiffer = true
}
} else if theirsChanged {
leftAndRightSchemasDiffer = true
diffInfo.LeftAndRightSchemasDiffer = true
// In this case, only theirsChanged, so we need to check if moving from ours->theirs
// is valid, otherwise it's a conflict
mergeInfo.LeftNeedsRewrite = true
Expand All @@ -467,7 +468,7 @@ func mergeColumns(tblName string, format *storetypes.NomsBinFormat, ourCC, their
})
}
} else if oursChanged {
leftAndRightSchemasDiffer = true
diffInfo.LeftAndRightSchemasDiffer = true
// In this case, only oursChanged, so we need to check if moving from theirs->ours
// is valid, otherwise it's a conflict
mergeInfo.RightNeedsRewrite = true
Expand All @@ -490,14 +491,14 @@ func mergeColumns(tblName string, format *storetypes.NomsBinFormat, ourCC, their
}
} else {
// The column was added on both branches.
ourSchemaChanged = true
theirSchemaChanged = true
diffInfo.LeftSchemaChange = true
diffInfo.RightSchemaChange = true
// If both columns changed in the same way, the modifications converge, so accept the column.
// If not, don't report a conflict, since this case is already handled in checkForColumnConflicts.
if ours.Equals(*theirs) {
mergedColumns = append(mergedColumns, *ours)
} else {
leftAndRightSchemasDiffer = true
diffInfo.LeftAndRightSchemasDiffer = true
}
}
}
Expand All @@ -506,10 +507,10 @@ 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, mergeInfo, nil
return nil, conflicts, mergeInfo, diffInfo, nil
}

return schema.NewColCollection(mergedColumns...), nil, mergeInfo, nil
return schema.NewColCollection(mergedColumns...), nil, mergeInfo, diffInfo, nil
}

// checkForColumnConflicts iterates over |mergedColumns|, checks for duplicate column names or column tags, and returns
Expand Down
2 changes: 1 addition & 1 deletion go/libraries/doltcore/merge/schema_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ 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")
_, 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))
Expand Down

0 comments on commit abcaea2

Please sign in to comment.