spanset: comment mergeSpans

Epic: none
Release note: none

Author: pav-kv

pkg/kv/kvserver/spanset/merge.go

@@ -10,6 +10,27 @@ import "slices"
 // mergeSpans sorts the given spans and merges ones with overlapping spans and
 // equal access timestamps. The implementation is a modified roachpb.MergeSpans.
 //
+// The input and output spans represent the same subset of {keys}x{timestamps}
+// space. The resulting spans are ordered by key. It is minimal (impossible to
+// "merge" further without losing precision) only if there are no keys present
+// under multiple timestamps.
+//
+// When there are keys present under multiple timestamps, the resulting merged
+// set is not guaranteed to be minimal. For example, [{a-b}@10, b@20, {b-c}@10]
+// is returned as is, even though we could merge it as [{a-c}@10, b@20]. This is
+// due to an arbitrary choice of sorting it by key.
+//
+// TODO(pav-kv): we could sort by (timestamp, key), which makes it possible to
+// merge spans with the same timestamp and guarantee minimality. After which, we
+// can optionally sort by key again. This is still more expensive than one sort,
+// so consider if the users are fine with the set sorted by (timestamp, key).
+//
+// The priority for this function is to be simple and free from assumptions
+// about how to "resolve" the situation of having the same key under multiple
+// timestamps. E.g. it does not assume that higher/lower timestamp "wins", or
+// what the semantics of an empty timestamp are. Considerations like that are
+// left to the consumer of this span set, e.g. the latch manager.
+//
 // The input spans are not safe for re-use.
 func mergeSpans(latches []Span) []Span {
 	if len(latches) == 0 {

pkg/kv/kvserver/spanset/spanset.go

@@ -212,7 +212,13 @@ func (s *SpanSet) Merge(s2 *SpanSet) {
 	s.SortAndDedup()
 }
 
-// SortAndDedup sorts the spans in the SpanSet and removes any duplicates.
+// SortAndDedup sorts the spans in the SpanSet by key and removes duplicate or
+// overlapping / redundant spans. The SpanSet represents the same subset of the
+// {keys}x{timestamps} space before and after this call, but is not guaranteed
+// to have a minimal number of spans in a general case (see mergeSpans comment).
+//
+// TODO(pav-kv): does this make CheckAllowed falsely fail in some cases? Maybe
+// it's fine: importantly, it should not falsely succeed.
 func (s *SpanSet) SortAndDedup() {
 	for sa := SpanAccess(0); sa < NumSpanAccess; sa++ {
 		for ss := SpanScope(0); ss < NumSpanScope; ss++ {