Skip to content

Commit

Permalink
Propagate considerAllRowsModified to more relevant places, and add …
Browse files Browse the repository at this point in the history 
…more meaningful comments and todos.
  • Loading branch information
@nicktobey
nicktobey committed Nov 28, 2023
1 parent 09d8c9d commit 60ccfec
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 16 deletions.
5 changes: 4 additions & 1 deletion go/libraries/doltcore/diff/diff_stat.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,10 @@ func diffProllyTrees(ctx context.Context, ch chan DiffStatProgress, keyless bool
}
}

err = prolly.DiffMaps(ctx, f, t, func(ctx context.Context, diff tree.Diff) error {
// TODO: Use `vMapping` to determine whether or not the schema has changed. If it has, then all rows should
// count as modifications in the diff.
considerAllRowsModified := false
err = prolly.DiffMaps(ctx, f, t, considerAllRowsModified, func(ctx context.Context, diff tree.Diff) error {
return rpr(ctx, vMapping, fVD, tVD, diff, ch)
})
if err != nil && err != io.EOF {
Expand Down
16 changes: 12 additions & 4 deletions go/libraries/doltcore/merge/violations_fk_prolly.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ func prollyParentSecDiffFkConstraintViolations(
childPriKD, _ := childPriIdx.Descriptors()

var err error
err = prolly.DiffMaps(ctx, preParentSecIdx, postParentSecIdx, func(ctx context.Context, diff tree.Diff) error {
// TODO: Determine whether we should surface every row as a diff when the map's value descriptor has changed.
considerAllRowsModified := false
err = prolly.DiffMaps(ctx, preParentSecIdx, postParentSecIdx, considerAllRowsModified, func(ctx context.Context, diff tree.Diff) error {
switch diff.Type {
case tree.RemovedDiff, tree.ModifiedDiff:
toSecKey, hadNulls := makePartialKey(partialKB, foreignKey.ReferencedTableColumns, postParent.Index, postParent.IndexSchema, val.Tuple(diff.Key), val.Tuple(diff.From), preParentSecIdx.Pool())
Expand Down Expand Up @@ -104,7 +106,9 @@ func prollyParentPriDiffFkConstraintViolations(
childScndryIdx := durable.ProllyMapFromIndex(postChild.IndexData)
primaryKD, _ := childPriIdx.Descriptors()

err := prolly.DiffMaps(ctx, preParentRowData, postParentRowData, func(ctx context.Context, diff tree.Diff) error {
// TODO: Determine whether we should surface every row as a diff when the map's value descriptor has changed.
considerAllRowsModified := false
err := prolly.DiffMaps(ctx, preParentRowData, postParentRowData, considerAllRowsModified, func(ctx context.Context, diff tree.Diff) error {
switch diff.Type {
case tree.RemovedDiff, tree.ModifiedDiff:
partialKey, hadNulls := makePartialKey(partialKB, foreignKey.ReferencedTableColumns, postParent.Index, postParent.Schema, val.Tuple(diff.Key), val.Tuple(diff.From), preParentRowData.Pool())
Expand Down Expand Up @@ -162,7 +166,9 @@ func prollyChildPriDiffFkConstraintViolations(
partialDesc := idxDesc.PrefixDesc(len(foreignKey.TableColumns))
partialKB := val.NewTupleBuilder(partialDesc)

err := prolly.DiffMaps(ctx, preChildRowData, postChildRowData, func(ctx context.Context, diff tree.Diff) error {
// TODO: Determine whether we should surface every row as a diff when the map's value descriptor has changed.
considerAllRowsModified := false
err := prolly.DiffMaps(ctx, preChildRowData, postChildRowData, considerAllRowsModified, func(ctx context.Context, diff tree.Diff) error {
switch diff.Type {
case tree.AddedDiff, tree.ModifiedDiff:
k, v := val.Tuple(diff.Key), val.Tuple(diff.To)
Expand Down Expand Up @@ -210,7 +216,9 @@ func prollyChildSecDiffFkConstraintViolations(
childPriKD, _ := postChildRowData.Descriptors()
childPriKB := val.NewTupleBuilder(childPriKD)

err := prolly.DiffMaps(ctx, preChildSecIdx, postChildSecIdx, func(ctx context.Context, diff tree.Diff) error {
// TODO: Determine whether we should surface every row as a diff when the map's value descriptor has changed.
considerAllRowsModified := false
err := prolly.DiffMaps(ctx, preChildSecIdx, postChildSecIdx, considerAllRowsModified, func(ctx context.Context, diff tree.Diff) error {
switch diff.Type {
case tree.AddedDiff, tree.ModifiedDiff:
k := val.Tuple(diff.Key)
Expand Down
4 changes: 3 additions & 1 deletion go/libraries/doltcore/sqle/dtables/diff_iter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,9 @@ func (itr prollyDiffIter) Close(ctx *sql.Context) error {
}

func (itr prollyDiffIter) queueRows(ctx context.Context) {
err := prolly.DiffMaps(ctx, itr.from, itr.to, func(ctx context.Context, d tree.Diff) error {
// TODO: Determine whether or not the schema has changed. If it has, then all rows should count as modifications in the diff.
considerAllRowsModified := false
err := prolly.DiffMaps(ctx, itr.from, itr.to, considerAllRowsModified, func(ctx context.Context, d tree.Diff) error {
dItr, err := itr.makeDiffRowItr(ctx, d)
if err != nil {
return err
Expand Down
16 changes: 8 additions & 8 deletions go/store/prolly/map_diff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func testMapDiffErrorHandling(t *testing.T, m Map) {
ctx := context.Background()

expErr := errors.New("error case")
err := DiffMaps(ctx, m, m, func(ctx context.Context, diff tree.Diff) error {
err := DiffMaps(ctx, m, m, false, func(ctx context.Context, diff tree.Diff) error {
return expErr
})
require.Error(t, expErr, err)
Expand All @@ -121,7 +121,7 @@ func testMapDiffErrorHandling(t *testing.T, m Map) {
func testEqualMapDiff(t *testing.T, m Map) {
ctx := context.Background()
var counter int
err := DiffMaps(ctx, m, m, func(ctx context.Context, diff tree.Diff) error {
err := DiffMaps(ctx, m, m, false, func(ctx context.Context, diff tree.Diff) error {
counter++
return nil
})
Expand All @@ -135,7 +135,7 @@ func testMapDiffAgainstEmpty(t *testing.T, scale int) {
empty, _ := makeProllyMap(t, 0)

cnt := 0
err := DiffMaps(ctx, m.(Map), empty.(Map), func(ctx context.Context, diff tree.Diff) error {
err := DiffMaps(ctx, m.(Map), empty.(Map), false, func(ctx context.Context, diff tree.Diff) error {
assert.Equal(t, tuples[cnt][0], val.Tuple(diff.Key))
assert.Equal(t, tuples[cnt][1], val.Tuple(diff.From))
assert.Nil(t, val.Tuple(diff.To))
Expand All @@ -146,7 +146,7 @@ func testMapDiffAgainstEmpty(t *testing.T, scale int) {
assert.Equal(t, scale, cnt)

cnt = 0
err = DiffMaps(ctx, empty.(Map), m.(Map), func(ctx context.Context, diff tree.Diff) error {
err = DiffMaps(ctx, empty.(Map), m.(Map), false, func(ctx context.Context, diff tree.Diff) error {
assert.Equal(t, tuples[cnt][0], val.Tuple(diff.Key))
assert.Equal(t, tuples[cnt][1], val.Tuple(diff.To))
assert.Nil(t, val.Tuple(diff.From))
Expand All @@ -170,7 +170,7 @@ func testDeleteDiffs(t *testing.T, from Map, tups [][2]val.Tuple, numDeletes int
to := makeMapWithDeletes(t, from, deletes...)

var cnt int
err := DiffMaps(ctx, from, to, func(ctx context.Context, diff tree.Diff) error {
err := DiffMaps(ctx, from, to, false, func(ctx context.Context, diff tree.Diff) error {
assert.Equal(t, tree.RemovedDiff, diff.Type)
assert.Equal(t, deletes[cnt][0], val.Tuple(diff.Key))
cnt++
Expand All @@ -185,7 +185,7 @@ func testInsertDiffs(t *testing.T, from Map, tups [][2]val.Tuple, numInserts int
to, inserts := makeMapWithInserts(t, from, numInserts)

var cnt int
err := DiffMaps(ctx, from, to, func(ctx context.Context, diff tree.Diff) error {
err := DiffMaps(ctx, from, to, false, func(ctx context.Context, diff tree.Diff) error {
if !assert.Equal(t, tree.AddedDiff, diff.Type) {
fmt.Println("")
}
Expand Down Expand Up @@ -215,7 +215,7 @@ func testUpdateDiffs(t *testing.T, from Map, tups [][2]val.Tuple, numUpdates int
to := makeMapWithUpdates(t, from, updates...)

var cnt int
err := DiffMaps(ctx, from, to, func(ctx context.Context, diff tree.Diff) error {
err := DiffMaps(ctx, from, to, false, func(ctx context.Context, diff tree.Diff) error {
assert.Equal(t, tree.ModifiedDiff, diff.Type)
assert.Equal(t, updates[cnt][0], val.Tuple(diff.Key))
assert.Equal(t, updates[cnt][1], val.Tuple(diff.From))
Expand All @@ -229,7 +229,7 @@ func testUpdateDiffs(t *testing.T, from Map, tups [][2]val.Tuple, numUpdates int

func testOneSidedDiff(t *testing.T, sz int, from, to Map, typ tree.DiffType) {
var seen int
err := DiffMaps(context.Background(), from, to, func(ctx context.Context, diff tree.Diff) error {
err := DiffMaps(context.Background(), from, to, false, func(ctx context.Context, diff tree.Diff) error {
assert.Equal(t, diff.Type, typ)
seen++
return nil
Expand Down
4 changes: 2 additions & 2 deletions go/store/prolly/tuple_map.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,8 @@ func MutateMapWithTupleIter(ctx context.Context, m Map, iter TupleIter) (Map, er
}, nil
}

func DiffMaps(ctx context.Context, from, to Map, cb tree.DiffFn) error {
return tree.DiffOrderedTrees(ctx, from.tuples, to.tuples, makeDiffCallBack(from, to, cb))
func DiffMaps(ctx context.Context, from, to Map, considerAllRowsModified bool, cb tree.DiffFn) error {
return tree.DiffOrderedTrees(ctx, from.tuples, to.tuples, considerAllRowsModified, makeDiffCallBack(from, to, cb))
}

// RangeDiffMaps returns diffs within a Range. See Range for which diffs are
Expand Down

0 comments on commit 60ccfec

Please sign in to comment.