keyspan: simplify Filter

The `FilterFunc` contract is not adequately spelled out; the code
makes certain unverified assumptions about how the bounds can be
changed.

Now that Truncate is a separate iterator, we no longer need Filter to
allow for changing bounds. We simplify `FilterFunc` to just return the
list of keys that are retained. We no longer need the extra
comparisons in `SeekGE/SeekLT`.

internal/keyspan/filter.go

@@ -6,13 +6,10 @@ package keyspan
 
 import "github.com/cockroachdb/pebble/internal/base"
 
-// FilterFunc defines a transform from the input Span into the output Span. The
-// function returns true if the Span should be returned by the iterator, and
-// false if the Span should be skipped. The FilterFunc is permitted to mutate
-// the output Span, for example, to elice certain keys, or update the Span's
-// bounds if so desired. The output Span's Keys slice may be reused to reduce
-// allocations.
-type FilterFunc func(in *Span, out *Span) (keep bool)
+// FilterFunc is a callback that allows filtering keys from a Span. The result
+// is the set of keys that should be retained (using buf as a buffer). If the
+// result has no keys, the span is skipped altogether.
+type FilterFunc func(span *Span, buf []Key) []Key
 
 // filteringIter is a FragmentIterator that uses a FilterFunc to select which
 // Spans from the input iterator are returned in the output.
@@ -45,17 +42,7 @@ func (i *filteringIter) SeekGE(key []byte) (*Span, error) {
 	if err != nil {
 		return nil, err
 	}
-	s, err = i.filter(s, +1)
-	if err != nil {
-		return nil, err
-	}
-	// i.filter could return a span that's less than key, _if_ the filterFunc
-	// (which has no knowledge of the seek key) mutated the span to end at a key
-	// less than or equal to `key`. Detect this case and next/invalidate the iter.
-	if s != nil && i.cmp(s.End, key) <= 0 {
-		return i.Next()
-	}
-	return s, nil
+	return i.filter(s, +1)
 }
 
 // SeekLT implements FragmentIterator.
@@ -64,17 +51,7 @@ func (i *filteringIter) SeekLT(key []byte) (*Span, error) {
 	if err != nil {
 		return nil, err
 	}
-	span, err = i.filter(span, -1)
-	if err != nil {
-		return nil, err
-	}
-	// i.filter could return a span that's >= key, _if_ the filterFunc (which has
-	// no knowledge of the seek key) mutated the span to start at a key greater
-	// than or equal to `key`. Detect this case and prev/invalidate the iter.
-	if span != nil && i.cmp(span.Start, key) >= 0 {
-		return i.Prev()
-	}
-	return span, nil
+	return i.filter(span, -1)
 }
 
 // First implements FragmentIterator.
@@ -128,9 +105,17 @@ func (i *filteringIter) filter(span *Span, dir int8) (*Span, error) {
 	}
 	var err error
 	for span != nil {
-		if keep := i.filterFn(span, &i.span); keep {
+		keys := i.filterFn(span, i.span.Keys[:0])
+		if len(keys) > 0 {
+			i.span = Span{
+				Start:     span.Start,
+				End:       span.End,
+				Keys:      keys,
+				KeysOrder: span.KeysOrder,
+			}
 			return &i.span, nil
 		}
+
 		if dir == +1 {
 			span, err = i.iter.Next()
 		} else {

internal/keyspan/filter_test.go

@@ -18,16 +18,15 @@ func TestFilteringIter(t *testing.T) {
 	// makeFilter returns a FilterFunc that will filter out all keys in a Span
 	// that are not of the given kind. Empty spans are skipped.
 	makeFilter := func(kind base.InternalKeyKind) FilterFunc {
-		return func(in *Span, out *Span) (keep bool) {
-			out.Start, out.End = in.Start, in.End
-			out.Keys = out.Keys[:0]
+		return func(in *Span, buf []Key) []Key {
+			keys := buf[:0]
 			for _, k := range in.Keys {
 				if k.Kind() != kind {
 					continue
 				}
-				out.Keys = append(out.Keys, k)
+				keys = append(keys, k)
 			}
-			return len(out.Keys) > 0
+			return keys
 		}
 	}
 

internal/keyspan/truncate.go

@@ -14,8 +14,9 @@ import (
 // truncated to be contained within the given user key bounds.
 //
 // Note that fragment iterator Spans always have exclusive end-keys; if the
-// given bounds have an inclusive end key the input iterator must not produce a
-// span that contains that key.
+// given bounds have an inclusive end key, then the input iterator must not
+// produce a span that contains that key. The only difference between bounds.End
+// being inclusive vs exclusive is this extra check.
 func Truncate(cmp base.Compare, iter FragmentIterator, bounds base.UserKeyBounds) FragmentIterator {
 	return &truncatingIter{
 		iter:   iter,
@@ -132,7 +133,7 @@ func (i *truncatingIter) nextSpanWithinBounds(
 			}
 			return nil, false, err
 		}
-		// Intersect [span.Start, span.End) with [lower, upper).
+		// Intersect [span.Start, span.End) with [i.bounds.Start, i.bounds.End.Key).
 		spanBoundsChanged = false
 		start := span.Start
 		if i.cmp(start, i.bounds.Start) < 0 {

table_stats.go

@@ -969,16 +969,15 @@ func newCombinedDeletionKeyspanIter(
 		}
 		// Wrap the range key iterator in a filter that elides keys other than range
 		// key deletions.
-		iter = keyspan.Filter(iter, func(in *keyspan.Span, out *keyspan.Span) (keep bool) {
-			out.Start, out.End = in.Start, in.End
-			out.Keys = out.Keys[:0]
+		iter = keyspan.Filter(iter, func(in *keyspan.Span, buf []keyspan.Key) []keyspan.Key {
+			keys := buf[:0]
 			for _, k := range in.Keys {
 				if k.Kind() != base.InternalKeyKindRangeKeyDelete {
 					continue
 				}
-				out.Keys = append(out.Keys, k)
+				keys = append(keys, k)
 			}
-			return len(out.Keys) > 0
+			return keys
 		}, comparer.Compare)
 		dIter := &keyspan.DefragmentingIter{}
 		dIter.Init(comparer, iter, equal, reducer, new(keyspan.DefragmentingBuffers))