From 934c0d0560bfca856deaa56a8988b3019871e9bd Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Wed, 25 Sep 2024 17:58:02 -0700 Subject: [PATCH 1/4] Use comparators from tuple descriptor for merges, not clear if these are all correct --- .../doltcore/merge/merge_prolly_rows.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/go/libraries/doltcore/merge/merge_prolly_rows.go b/go/libraries/doltcore/merge/merge_prolly_rows.go index 54036ff047..7a6d839675 100644 --- a/go/libraries/doltcore/merge/merge_prolly_rows.go +++ b/go/libraries/doltcore/merge/merge_prolly_rows.go @@ -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 isEqualComparator(m.baseVD.Comparator(), i, baseCol, rightCol, m.rightVD.Types[rightColIdx]) { // right column did not change, so there is no conflict. return false, nil } @@ -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 isEqualComparator(m.leftVD.Comparator(), i, baseCol, leftCol, m.leftVD.Types[leftColIdx]) { // left column did not change, so there is no conflict. return false, nil } @@ -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 isEqualComparator(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. @@ -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 = !isEqualComparator(m.rightVD.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 isEqualComparator(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. @@ -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 = !isEqualComparator(m.baseVD.Comparator(), i, leftCol, baseCol, resultType) switch { case leftModified && rightModified: @@ -2141,6 +2141,12 @@ func isEqual(i int, left []byte, right []byte, resultType val.Type) bool { 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. + return cmp.CompareValues(i, left, right, resultType) == 0 +} + func getColumn(tuple *val.Tuple, mapping *val.OrdinalMapping, idx int) (col []byte, colIndex int, exists bool) { colIdx := (*mapping)[idx] if colIdx == -1 { From 35194d0d72f53763be9077e57bdcc9be3c137c5b Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 26 Sep 2024 11:51:35 -0700 Subject: [PATCH 2/4] More tablename plumbing --- .../sqle/dtables/conflicts_tables_prolly.go | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/go/libraries/doltcore/sqle/dtables/conflicts_tables_prolly.go b/go/libraries/doltcore/sqle/dtables/conflicts_tables_prolly.go index e1b0c475a3..e25320ae95 100644 --- a/go/libraries/doltcore/sqle/dtables/conflicts_tables_prolly.go +++ b/go/libraries/doltcore/sqle/dtables/conflicts_tables_prolly.go @@ -15,7 +15,6 @@ package dtables import ( - "context" "encoding/base64" "fmt" @@ -64,7 +63,7 @@ func newProllyConflictsTable( } return ProllyConflictsTable{ - tblName: tblName.Name, + tblName: tblName, sqlSch: sqlSch, baseSch: baseSch, ourSch: ourSch, @@ -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 @@ -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 { @@ -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 @@ -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 } @@ -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 } @@ -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 } From a7bcb239bc2aa4525faddbd8722f4154e3fb630b Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 26 Sep 2024 16:08:21 -0700 Subject: [PATCH 3/4] Fixed a panic in outputting prolly nodes under some circumstances. Fixed 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. --- .../doltcore/merge/merge_prolly_rows.go | 24 +++++++------------ .../doltcore/merge/schema_merge_test.go | 2 +- go/store/prolly/tree/node.go | 2 +- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/go/libraries/doltcore/merge/merge_prolly_rows.go b/go/libraries/doltcore/merge/merge_prolly_rows.go index 7a6d839675..536b87eb6a 100644 --- a/go/libraries/doltcore/merge/merge_prolly_rows.go +++ b/go/libraries/doltcore/merge/merge_prolly_rows.go @@ -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 } @@ -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 } @@ -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. @@ -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. @@ -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. @@ -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: @@ -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 } diff --git a/go/libraries/doltcore/merge/schema_merge_test.go b/go/libraries/doltcore/merge/schema_merge_test.go index d93db694c4..1abe6e1c66 100644 --- a/go/libraries/doltcore/merge/schema_merge_test.go +++ b/go/libraries/doltcore/merge/schema_merge_test.go @@ -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", diff --git a/go/store/prolly/tree/node.go b/go/store/prolly/tree/node.go index 8077f06f08..73e185dce6 100644 --- a/go/store/prolly/tree/node.go +++ b/go/store/prolly/tree/node.go @@ -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("#")) } From 90ef3cab7317233aa5678eb208feecc75a2d65b8 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 26 Sep 2024 17:08:49 -0700 Subject: [PATCH 4/4] rename test --- go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go | 2 +- go/libraries/doltcore/sqle/enginetest/dolt_engine_tests.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go b/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go index 53ce6671d4..13f7a75e35 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go @@ -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 diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_engine_tests.go b/go/libraries/doltcore/sqle/enginetest/dolt_engine_tests.go index 4e7f268946..b82a05ad88 100755 --- a/go/libraries/doltcore/sqle/enginetest/dolt_engine_tests.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_engine_tests.go @@ -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() }