Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the a type comparator for merge, not the generic comparator #8399

Merged
merged 4 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 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 isEqual(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 isEqual(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 isEqual(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 = !isEqual(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 isEqual(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 = !isEqual(i, leftCol, baseCol, resultType)
leftModified = !isEqual(m.resultVD.Comparator(), i, leftCol, baseCol, resultType)

switch {
case leftModified && rightModified:
Expand Down Expand Up @@ -2135,10 +2135,8 @@ 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 isEqual(cmp val.TupleComparator, i int, left []byte, right []byte, resultType val.Type) bool {
return cmp.CompareValues(i, left, right, resultType) == 0
}

func getColumn(tuple *val.Tuple, mapping *val.OrdinalMapping, idx int) (col []byte, colIndex int, exists bool) {
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
20 changes: 10 additions & 10 deletions go/libraries/doltcore/sqle/dtables/conflicts_tables_prolly.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package dtables

import (
"context"
"encoding/base64"
"fmt"

Expand Down Expand Up @@ -64,7 +63,7 @@ func newProllyConflictsTable(
}

return ProllyConflictsTable{
tblName: tblName.Name,
tblName: tblName,
sqlSch: sqlSch,
baseSch: baseSch,
ourSch: ourSch,
Expand All @@ -81,7 +80,7 @@ func newProllyConflictsTable(
// ProllyConflictsTable is a sql.Table implementation that uses the merge
// artifacts table to persist and read conflicts.
type ProllyConflictsTable struct {
tblName string
tblName doltdb.TableName
sqlSch sql.PrimaryKeySchema
baseSch, ourSch, theirSch schema.Schema
root doltdb.RootValue
Expand All @@ -96,11 +95,11 @@ var _ sql.UpdatableTable = ProllyConflictsTable{}
var _ sql.DeletableTable = ProllyConflictsTable{}

func (ct ProllyConflictsTable) Name() string {
return doltdb.DoltConfTablePrefix + ct.tblName
return doltdb.DoltConfTablePrefix + ct.tblName.Name
}

func (ct ProllyConflictsTable) String() string {
return doltdb.DoltConfTablePrefix + ct.tblName
return doltdb.DoltConfTablePrefix + ct.tblName.Name
}

func (ct ProllyConflictsTable) Schema() sql.Schema {
Expand Down Expand Up @@ -130,7 +129,7 @@ func (ct ProllyConflictsTable) Deleter(ctx *sql.Context) sql.RowDeleter {

type prollyConflictRowIter struct {
itr prolly.ConflictArtifactIter
tblName string
tblName doltdb.TableName
vrw types.ValueReadWriter
ns tree.NodeStore
ourRows prolly.Map
Expand Down Expand Up @@ -404,13 +403,13 @@ func (itr *prollyConflictRowIter) nextConflictVals(ctx *sql.Context) (c conf, er

// loadTableMaps loads the maps specified in the metadata if they are different from
// the currently loaded maps. |baseHash| and |theirHash| are table hashes.
func (itr *prollyConflictRowIter) loadTableMaps(ctx context.Context, baseHash, theirHash hash.Hash) error {
func (itr *prollyConflictRowIter) loadTableMaps(ctx *sql.Context, baseHash, theirHash hash.Hash) error {
if itr.baseHash.Compare(baseHash) != 0 {
rv, err := doltdb.LoadRootValueFromRootIshAddr(ctx, itr.vrw, itr.ns, baseHash)
if err != nil {
return err
}
baseTbl, ok, err := rv.GetTable(ctx, doltdb.TableName{Name: itr.tblName})
baseTbl, ok, err := rv.GetTable(ctx, itr.tblName)
if err != nil {
return err
}
Expand All @@ -435,7 +434,8 @@ func (itr *prollyConflictRowIter) loadTableMaps(ctx context.Context, baseHash, t
if err != nil {
return err
}
theirTbl, ok, err := rv.GetTable(ctx, doltdb.TableName{Name: itr.tblName})

theirTbl, ok, err := rv.GetTable(ctx, itr.tblName)
if err != nil {
return err
}
Expand Down Expand Up @@ -660,7 +660,7 @@ func (cd *prollyConflictDeleter) Close(ctx *sql.Context) error {
return err
}

updatedRoot, err := cd.ct.root.PutTable(ctx, doltdb.TableName{Name: cd.ct.tblName}, updatedTbl)
updatedRoot, err := cd.ct.root.PutTable(ctx, cd.ct.tblName, updatedTbl)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1292,7 +1292,7 @@ func TestDoltConflictsTableNameTable(t *testing.T) {
// tests new format behavior for keyless merges that create CVs and conflicts
func TestKeylessDoltMergeCVsAndConflicts(t *testing.T) {
h := newDoltEnginetestHarness(t)
RunKelyessDoltMergeCVsAndConflictsTests(t, h)
RunKeylessDoltMergeCVsAndConflictsTests(t, h)
}

// eventually this will be part of TestDoltMerge
Expand Down
2 changes: 1 addition & 1 deletion go/libraries/doltcore/sqle/enginetest/dolt_engine_tests.go
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,7 @@ func RunDoltConflictsTableNameTableTests(t *testing.T, h DoltEnginetestHarness)
}
}

func RunKelyessDoltMergeCVsAndConflictsTests(t *testing.T, h DoltEnginetestHarness) {
func RunKeylessDoltMergeCVsAndConflictsTests(t *testing.T, h DoltEnginetestHarness) {
if !types.IsFormat_DOLT(types.Format_Default) {
t.Skip()
}
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
Loading