Propagate considerAllRowsModified to more relevant places, and add …

…more meaningful comments and todos.

go/store/prolly/artifact_map.go

@@ -296,7 +296,13 @@ func (m ArtifactMap) iterAllOfTypes(ctx context.Context, artTypes ...ArtifactTyp
 
 func MergeArtifactMaps(ctx context.Context, left, right, base ArtifactMap, cb tree.CollisionFn) (ArtifactMap, error) {
 	serializer := message.NewMergeArtifactSerializer(base.keyDesc, left.tuples.NodeStore.Pool())
-	tuples, _, err := tree.MergeOrderedTrees(ctx, left.tuples, right.tuples, base.tuples, cb, serializer)
+	// TODO: MergeArtifactMaps does not properly detect merge conflicts when one side adds a NULL to the end of its tuple.
+	// To fix this, accurate values of `leftSchemaChanged` and `rightSchemaChanged` must be computed.
+	// However, currently we do not expect the value of ArtifactMap.valDesc to be different across branches,
+	// so we can safely assume `false` for both parameters.
+	leftSchemaChanged := false
+	rightSchemaChanged := false
+	tuples, _, err := tree.MergeOrderedTrees(ctx, left.tuples, right.tuples, base.tuples, cb, leftSchemaChanged, rightSchemaChanged, serializer)
 	if err != nil {
 		return ArtifactMap{}, err
 	}

go/store/prolly/tree/map.go

@@ -40,10 +40,10 @@ type StaticMap[K, V ~[]byte, O Ordering[K]] struct {
 func DiffOrderedTrees[K, V ~[]byte, O Ordering[K]](
 	ctx context.Context,
 	from, to StaticMap[K, V, O],
+	considerAllRowsModified bool,
 	cb DiffFn,
 ) error {
-	// TODO: If the schema has changed, do we want every row to be considered changed?
-	differ, err := DifferFromRoots[K](ctx, from.NodeStore, to.NodeStore, from.Root, to.Root, from.Order, false)
+	differ, err := DifferFromRoots[K](ctx, from.NodeStore, to.NodeStore, from.Root, to.Root, from.Order, considerAllRowsModified)
 	if err != nil {
 		return err
 	}
@@ -139,9 +139,10 @@ func MergeOrderedTrees[K, V ~[]byte, O Ordering[K], S message.Serializer](
 	ctx context.Context,
 	l, r, base StaticMap[K, V, O],
 	cb CollisionFn,
+	leftSchemaChanged, rightSchemaChanged bool,
 	serializer S,
 ) (StaticMap[K, V, O], MergeStats, error) {
-	root, stats, err := ThreeWayMerge[K](ctx, base.NodeStore, l.Root, r.Root, base.Root, cb, base.Order, serializer)
+	root, stats, err := ThreeWayMerge[K](ctx, base.NodeStore, l.Root, r.Root, base.Root, cb, leftSchemaChanged, rightSchemaChanged, base.Order, serializer)
 	if err != nil {
 		return StaticMap[K, V, O]{}, MergeStats{}, err
 	}

go/store/prolly/tree/merge.go

@@ -47,18 +47,16 @@ func ThreeWayMerge[K ~[]byte, O Ordering[K], S message.Serializer](
 	ns NodeStore,
 	left, right, base Node,
 	collide CollisionFn,
+	leftSchemaChange, rightSchemaChange bool,
 	order O,
 	serializer S,
 ) (final Node, stats MergeStats, err error) {
-
-	// TODO: This function is only ever called to merge artifacts. Is is possible for the tuple desc to change here?
-	// If it does, do we want every row to appear in the diff?
-	ld, err := DifferFromRoots[K](ctx, ns, ns, base, left, order, false)
+	ld, err := DifferFromRoots[K](ctx, ns, ns, base, left, order, leftSchemaChange)
 	if err != nil {
 		return Node{}, MergeStats{}, err
 	}
 
-	rd, err := DifferFromRoots[K](ctx, ns, ns, base, right, order, false)
+	rd, err := DifferFromRoots[K](ctx, ns, ns, base, right, order, rightSchemaChange)
 	if err != nil {
 		return Node{}, MergeStats{}, err
 	}

go/store/prolly/tuple_map.go

@@ -112,7 +112,9 @@ func MutateMapWithTupleIter(ctx context.Context, m Map, iter TupleIter) (Map, er
 }
 
 func DiffMaps(ctx context.Context, from, to Map, cb tree.DiffFn) error {
-	return tree.DiffOrderedTrees(ctx, from.tuples, to.tuples, makeDiffCallBack(from, to, cb))
+	// TODO: If a key with a null value was added or removed from the end of the map, the diff may not detect it.
+	considerAllRowsModified := false
+	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
@@ -168,7 +170,12 @@ func makeDiffCallBack(from, to Map, innerCb tree.DiffFn) tree.DiffFn {
 
 func MergeMaps(ctx context.Context, left, right, base Map, cb tree.CollisionFn) (Map, tree.MergeStats, error) {
 	serializer := message.NewProllyMapSerializer(left.valDesc, base.NodeStore().Pool())
-	tuples, stats, err := tree.MergeOrderedTrees(ctx, left.tuples, right.tuples, base.tuples, cb, serializer)
+	// TODO: MergeMaps does not properly detect merge conflicts when one side adds a NULL to the end of its tuple.
+	// To fix this, accurate values of `leftSchemaChanged` and `rightSchemaChanged` must be computed.
+	// However, since `MergeMaps` is not currently called, fixing this is not a priority.
+	leftSchemaChanged := false
+	rightSchemaChanged := false
+	tuples, stats, err := tree.MergeOrderedTrees(ctx, left.tuples, right.tuples, base.tuples, cb, leftSchemaChanged, rightSchemaChanged, serializer)
 	if err != nil {
 		return Map{}, tree.MergeStats{}, err
 	}