Skip to content

Commit

Permalink
Fixed a panic in outputting prolly nodes under some circumstances. Fi…
Browse files Browse the repository at this point in the history
…xed a test error where collated strings with equivalent but not byte-identical column values were taking the lower value on merge, rather than the higher value like other types.
  • Loading branch information
zachmu committed Sep 26, 2024
1 parent 35194d0 commit a7bcb23
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 18 deletions.
24 changes: 8 additions & 16 deletions go/libraries/doltcore/merge/merge_prolly_rows.go
Original file line number Diff line number Diff line change
Expand Up @@ -1766,7 +1766,7 @@ func (m *valueMerger) processBaseColumn(ctx context.Context, i int, left, right,
if err != nil {
return false, err
}
if isEqualComparator(m.baseVD.Comparator(), i, baseCol, rightCol, m.rightVD.Types[rightColIdx]) {
if isEqual(m.baseVD.Comparator(), i, baseCol, rightCol, m.rightVD.Types[rightColIdx]) {
// right column did not change, so there is no conflict.
return false, nil
}
Expand All @@ -1789,7 +1789,7 @@ func (m *valueMerger) processBaseColumn(ctx context.Context, i int, left, right,
if err != nil {
return false, err
}
if isEqualComparator(m.leftVD.Comparator(), i, baseCol, leftCol, m.leftVD.Types[leftColIdx]) {
if isEqual(m.baseVD.Comparator(), i, baseCol, leftCol, m.leftVD.Types[leftColIdx]) {
// left column did not change, so there is no conflict.
return false, nil
}
Expand Down Expand Up @@ -1880,7 +1880,7 @@ func (m *valueMerger) processColumn(ctx *sql.Context, i int, left, right, base v
return nil, false, err
}

if isEqualComparator(m.leftVD.Comparator(), i, leftCol, rightCol, resultType) {
if isEqual(m.leftVD.Comparator(), i, leftCol, rightCol, resultType) {
// Columns are equal, returning either would be correct.
// However, for certain types the two columns may have different bytes.
// We need to ensure that merges are deterministic regardless of the merge direction.
Expand All @@ -1900,7 +1900,7 @@ func (m *valueMerger) processColumn(ctx *sql.Context, i int, left, right, base v
return nil, true, nil
}

// We can now assume that both left are right contain byte-level changes to an existing column.
// We can now assume that both left and right contain byte-level changes to an existing column.
// But we need to know if those byte-level changes represent a modification to the underlying value,
// and whether those changes represent the *same* modification, otherwise there's a conflict.

Expand Down Expand Up @@ -1934,14 +1934,14 @@ func (m *valueMerger) processColumn(ctx *sql.Context, i int, left, right, base v
if err != nil {
return nil, true, nil
}
rightModified = !isEqualComparator(m.rightVD.Comparator(), i, rightCol, baseCol, resultType)
rightModified = !isEqual(m.resultVD.Comparator(), i, rightCol, baseCol, resultType)
}

leftCol, err = convert(ctx, m.leftVD, m.resultVD, m.resultSchema, leftColIdx, i, left, leftCol, m.ns)
if err != nil {
return nil, true, nil
}
if isEqualComparator(m.leftVD.Comparator(), i, leftCol, rightCol, resultType) {
if isEqual(m.resultVD.Comparator(), i, leftCol, rightCol, resultType) {
// Columns are equal, returning either would be correct.
// However, for certain types the two columns may have different bytes.
// We need to ensure that merges are deterministic regardless of the merge direction.
Expand All @@ -1952,7 +1952,7 @@ func (m *valueMerger) processColumn(ctx *sql.Context, i int, left, right, base v
return rightCol, false, nil
}

leftModified = !isEqualComparator(m.baseVD.Comparator(), i, leftCol, baseCol, resultType)
leftModified = !isEqual(m.resultVD.Comparator(), i, leftCol, baseCol, resultType)

switch {
case leftModified && rightModified:
Expand Down Expand Up @@ -2135,15 +2135,7 @@ func mergeJSON(ctx context.Context, ns tree.NodeStore, base, left, right sql.JSO
}
}

func isEqual(i int, left []byte, right []byte, resultType val.Type) bool {
// We use a default comparator instead of the comparator in the schema.
// This is necessary to force a binary collation for string comparisons.
return val.DefaultTupleComparator{}.CompareValues(i, left, right, resultType) == 0
}

func isEqualComparator(cmp val.TupleComparator, i int, left []byte, right []byte, resultType val.Type) bool {
// We use a default comparator instead of the comparator in the schema.
// This is necessary to force a binary collation for string comparisons.
func isEqual(cmp val.TupleComparator, i int, left []byte, right []byte, resultType val.Type) bool {
return cmp.CompareValues(i, left, right, resultType) == 0
}

Expand Down
2 changes: 1 addition & 1 deletion go/libraries/doltcore/merge/schema_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ var collationTests = []schemaMergeTest{
ancestor: singleRow(1, 1, 1, "foo", decimal.New(100, 0)),
left: singleRow(1, 1, 2, "FOO", decimal.New(100, 0)),
right: singleRow(1, 2, 1, "foo", decimal.New(100, 0)),
merged: singleRow(1, 2, 2, "FOO", decimal.New(100, 0)),
merged: singleRow(1, 2, 2, "foo", decimal.New(100, 0)),
},
{
name: "conflict removal and replace varchar with equal replacement",
Expand Down
2 changes: 1 addition & 1 deletion go/store/prolly/tree/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func OutputProllyNode(ctx context.Context, w io.Writer, node Node, ns NodeStore,
w.Write([]byte(", "))
}

isAddr := val.IsAddrEncoding(kd.Types[j].Enc)
isAddr := j < len(kd.Types) && val.IsAddrEncoding(kd.Types[j].Enc)
if isAddr {
w.Write([]byte("#"))
}
Expand Down

0 comments on commit a7bcb23

Please sign in to comment.