Repository: cockroachdb/pebble
Pull request #3789 (2024-07-25 17:27:03)

sstable: clean up point iterator constructors
#### sstable: improve TrivialReaderProvider

We improve the `TrivialReaderProvider` implementation to avoid
allocation and unexport the type.

#### sstable: remove NewIterWithBlockPropertyFilters


#### sstable: remove compaction iterator wrappers

The `compactionIterator` and `twoLevelCompactionIterator` wrappers no
longer serve any purpose.

#### sstable: rename NewIterWithBlockPropertyFiltersAndContextEtc

Rename to `NewPointIter`.

cockroach-teamcity: 

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/cockroachdb/pebble/3789)
<!-- Reviewable:end -->


Patch:
diff --git a/external_iterator.go b/external_iterator.go
index e667e910..08eecca0 100644
--- a/external_iterator.go
+++ b/external_iterator.go
@@ -156,11 +156,11 @@ func createExternalPointIter(ctx context.Context, it *Iterator) (topLevelIterato
 			// BlockPropertiesFilterer that includes obsoleteKeyBlockPropertyFilter.
 			transforms := sstable.IterTransforms{SyntheticSeqNum: sstable.SyntheticSeqNum(seqNum)}
 			seqNum--
-			pointIter, err = r.NewIterWithBlockPropertyFiltersAndContextEtc(
+			pointIter, err = r.NewPointIter(
 				ctx, transforms, it.opts.LowerBound, it.opts.UpperBound, nil, /* BlockPropertiesFilterer */
 				sstable.NeverUseFilterBlock,
 				&it.stats.InternalStats, it.opts.CategoryAndQoS, nil,
-				sstable.TrivialReaderProvider{Reader: r})
+				sstable.MakeTrivialReaderProvider(r))
 			if err != nil {
 				return nil, err
 			}
diff --git a/sstable/reader.go b/sstable/reader.go
index 8ccd4e88..afec311f 100644
--- a/sstable/reader.go
+++ b/sstable/reader.go
@@ -151,32 +151,12 @@ func (r *Reader) Close() error {
 	return nil
 }
 
-// NewIterWithBlockPropertyFilters returns an iterator for the contents of the
-// table. If an error occurs, NewIterWithBlockPropertyFilters cleans up after
-// itself and returns a nil iterator.
-func (r *Reader) NewIterWithBlockPropertyFilters(
-	transforms IterTransforms,
-	lower, upper []byte,
-	filterer *BlockPropertiesFilterer,
-	filterBlockSizeLimit FilterBlockSizeLimit,
-	stats *base.InternalIteratorStats,
-	categoryAndQoS CategoryAndQoS,
-	statsCollector *CategoryStatsCollector,
-	rp ReaderProvider,
-) (Iterator, error) {
-	return r.newIterWithBlockPropertyFiltersAndContext(
-		context.Background(), transforms, lower, upper, filterer, filterBlockSizeLimit,
-		stats, categoryAndQoS, statsCollector, rp, nil)
-}
-
-// NewIterWithBlockPropertyFiltersAndContextEtc is similar to
-// NewIterWithBlockPropertyFilters and additionally accepts a context for
-// tracing.
+// NewPointIter returns an iterator for the point keys in the table.
 //
 // If transform.HideObsoletePoints is set, the callee assumes that filterer
 // already includes obsoleteKeyBlockPropertyFilter. The caller can satisfy this
 // contract by first calling TryAddBlockPropertyFilterForHideObsoletePoints.
-func (r *Reader) NewIterWithBlockPropertyFiltersAndContextEtc(
+func (r *Reader) NewPointIter(
 	ctx context.Context,
 	transforms IterTransforms,
 	lower, upper []byte,
@@ -187,14 +167,14 @@ func (r *Reader) NewIterWithBlockPropertyFiltersAndContextEtc(
 	statsCollector *CategoryStatsCollector,
 	rp ReaderProvider,
 ) (Iterator, error) {
-	return r.newIterWithBlockPropertyFiltersAndContext(
+	return r.newPointIter(
 		ctx, transforms, lower, upper, filterer, filterBlockSizeLimit,
 		stats, categoryAndQoS, statsCollector, rp, nil)
 }
 
 // TryAddBlockPropertyFilterForHideObsoletePoints is expected to be called
-// before the call to NewIterWithBlockPropertyFiltersAndContextEtc, to get the
-// value of hideObsoletePoints and potentially add a block property filter.
+// before the call to NewPointIter, to get the value of hideObsoletePoints and
+// potentially add a block property filter.
 func (r *Reader) TryAddBlockPropertyFilterForHideObsoletePoints(
 	snapshotForHideObsoletePoints base.SeqNum,
 	fileLargestSeqNum base.SeqNum,
@@ -208,7 +188,7 @@ func (r *Reader) TryAddBlockPropertyFilterForHideObsoletePoints(
 	return hideObsoletePoints, pointKeyFilters
 }
 
-func (r *Reader) newIterWithBlockPropertyFiltersAndContext(
+func (r *Reader) newPointIter(
 	ctx context.Context,
 	transforms IterTransforms,
 	lower, upper []byte,
@@ -241,16 +221,18 @@ func (r *Reader) newIterWithBlockPropertyFiltersAndContext(
 	return res, nil
 }
 
-// NewIter returns an iterator for the contents of the table. If an error
-// occurs, NewIter cleans up after itself and returns a nil iterator. NewIter
-// must only be used when the Reader is guaranteed to outlive any LazyValues
-// returned from the iter.
+// NewIter returns an iterator for the point keys in the table. It is a
+// simplified version of NewPointIter and should only be used for tests and
+// tooling.
+//
+// NewIter must only be used when the Reader is guaranteed to outlive any
+// LazyValues returned from the iter.
 func (r *Reader) NewIter(transforms IterTransforms, lower, upper []byte) (Iterator, error) {
 	// TODO(radu): we should probably not use bloom filters in this case, as there
 	// likely isn't a cache set up.
-	return r.NewIterWithBlockPropertyFilters(
-		transforms, lower, upper, nil, AlwaysUseFilterBlock,
-		nil /* stats */, CategoryAndQoS{}, nil /* statsCollector */, TrivialReaderProvider{Reader: r})
+	return r.NewPointIter(
+		context.TODO(), transforms, lower, upper, nil, AlwaysUseFilterBlock,
+		nil /* stats */, CategoryAndQoS{}, nil /* statsCollector */, MakeTrivialReaderProvider(r))
 }
 
 // NewCompactionIter returns an iterator similar to NewIter but it also increments
@@ -287,7 +269,7 @@ func (r *Reader) newCompactionIter(
 			return nil, err
 		}
 		i.setupForCompaction()
-		return &twoLevelCompactionIterator{twoLevelIterator: i}, nil
+		return i, nil
 	}
 	i, err := newSingleLevelIterator(
 		context.Background(), r, vState, transforms, nil /* lower */, nil, /* upper */
@@ -297,7 +279,7 @@ func (r *Reader) newCompactionIter(
 		return nil, err
 	}
 	i.setupForCompaction()
-	return &compactionIterator{singleLevelIterator: i}, nil
+	return i, nil
 }
 
 // NewRawRangeDelIter returns an internal iterator for the contents of the
diff --git a/sstable/reader_common.go b/sstable/reader_common.go
index 7158c910..0587110a 100644
--- a/sstable/reader_common.go
+++ b/sstable/reader_common.go
@@ -25,7 +25,7 @@ type CommonReader interface {
 		ctx context.Context, transforms FragmentIterTransforms,
 	) (keyspan.FragmentIterator, error)
 
-	NewIterWithBlockPropertyFiltersAndContextEtc(
+	NewPointIter(
 		ctx context.Context,
 		transforms IterTransforms,
 		lower, upper []byte,
diff --git a/sstable/reader_iter.go b/sstable/reader_iter.go
index 52cae69a..d3397448 100644
--- a/sstable/reader_iter.go
+++ b/sstable/reader_iter.go
@@ -149,100 +149,3 @@ func checkTwoLevelIterator(obj interface{}) {
 		os.Exit(1)
 	}
 }
-
-// compactionIterator is similar to Iterator but it increments the number of
-// bytes that have been iterated through.
-type compactionIterator struct {
-	*singleLevelIterator
-}
-
-// compactionIterator implements the base.InternalIterator interface.
-var _ base.InternalIterator = (*compactionIterator)(nil)
-
-func (i *compactionIterator) String() string {
-	if i.vState != nil {
-		return i.vState.fileNum.String()
-	}
-	return i.reader.cacheOpts.FileNum.String()
-}
-
-func (i *compactionIterator) SeekGE(key []byte, flags base.SeekGEFlags) *base.InternalKV {
-	panic("pebble: SeekGE unimplemented")
-}
-
-func (i *compactionIterator) SeekPrefixGE(
-	prefix, key []byte, flags base.SeekGEFlags,
-) *base.InternalKV {
-	panic("pebble: SeekPrefixGE unimplemented")
-}
-
-func (i *compactionIterator) SeekLT(key []byte, flags base.SeekLTFlags) *base.InternalKV {
-	panic("pebble: SeekLT unimplemented")
-}
-
-func (i *compactionIterator) First() *base.InternalKV {
-	i.err = nil // clear cached iteration error
-	return i.skipForward(i.singleLevelIterator.First())
-}
-
-func (i *compactionIterator) Last() *base.InternalKV {
-	panic("pebble: Last unimplemented")
-}
-
-// Note: compactionIterator.Next mirrors the implementation of Iterator.Next
-// due to performance. Keep the two in sync.
-func (i *compactionIterator) Next() *base.InternalKV {
-	if i.err != nil {
-		return nil
-	}
-	return i.skipForward(i.data.Next())
-}
-
-func (i *compactionIterator) NextPrefix(succKey []byte) *base.InternalKV {
-	panic("pebble: NextPrefix unimplemented")
-}
-
-func (i *compactionIterator) Prev() *base.InternalKV {
-	panic("pebble: Prev unimplemented")
-}
-
-func (i *compactionIterator) skipForward(kv *base.InternalKV) *base.InternalKV {
-	if kv == nil {
-		for {
-			if kv := i.index.Next(); kv == nil {
-				break
-			}
-			result := i.loadBlock(+1)
-			if result != loadBlockOK {
-				if i.err != nil {
-					break
-				}
-				switch result {
-				case loadBlockFailed:
-					// We checked that i.index was at a valid entry, so
-					// loadBlockFailed could not have happened due to to i.index
-					// being exhausted, and must be due to an error.
-					panic("loadBlock should not have failed with no error")
-				case loadBlockIrrelevant:
-					panic("compactionIter should not be using block intervals for skipping")
-				default:
-					panic(fmt.Sprintf("unexpected case %d", result))
-				}
-			}
-			// result == loadBlockOK
-			if kv = i.data.First(); kv != nil {
-				break
-			}
-		}
-	}
-
-	// We have an upper bound when the table is virtual.
-	if i.upper != nil && kv != nil {
-		cmp := i.cmp(kv.K.UserKey, i.upper)
-		if cmp > 0 || (!i.endKeyInclusive && cmp == 0) {
-			return nil
-		}
-	}
-
-	return kv
-}
diff --git a/sstable/reader_iter_two_lvl.go b/sstable/reader_iter_two_lvl.go
index 1ebf9b68..8ad7419f 100644
--- a/sstable/reader_iter_two_lvl.go
+++ b/sstable/reader_iter_two_lvl.go
@@ -964,97 +964,3 @@ func (i *twoLevelIterator) Close() error {
 	twoLevelIterPool.Put(i)
 	return err
 }
-
-// Note: twoLevelCompactionIterator and compactionIterator are very similar but
-// were separated due to performance.
-type twoLevelCompactionIterator struct {
-	*twoLevelIterator
-}
-
-// twoLevelCompactionIterator implements the base.InternalIterator interface.
-var _ base.InternalIterator = (*twoLevelCompactionIterator)(nil)
-
-func (i *twoLevelCompactionIterator) Close() error {
-	return i.twoLevelIterator.Close()
-}
-
-func (i *twoLevelCompactionIterator) SeekGE(key []byte, flags base.SeekGEFlags) *base.InternalKV {
-	panic("pebble: SeekGE unimplemented")
-}
-
-func (i *twoLevelCompactionIterator) SeekPrefixGE(
-	prefix, key []byte, flags base.SeekGEFlags,
-) *base.InternalKV {
-	panic("pebble: SeekPrefixGE unimplemented")
-}
-
-func (i *twoLevelCompactionIterator) SeekLT(key []byte, flags base.SeekLTFlags) *base.InternalKV {
-	panic("pebble: SeekLT unimplemented")
-}
-
-func (i *twoLevelCompactionIterator) First() *base.InternalKV {
-	i.err = nil // clear cached iteration error
-	return i.skipForward(i.twoLevelIterator.First())
-}
-
-func (i *twoLevelCompactionIterator) Last() *base.InternalKV {
-	panic("pebble: Last unimplemented")
-}
-
-// Note: twoLevelCompactionIterator.Next mirrors the implementation of
-// twoLevelIterator.Next due to performance. Keep the two in sync.
-func (i *twoLevelCompactionIterator) Next() *base.InternalKV {
-	if i.err != nil {
-		return nil
-	}
-	return i.skipForward(i.singleLevelIterator.Next())
-}
-
-func (i *twoLevelCompactionIterator) NextPrefix(succKey []byte) *base.InternalKV {
-	panic("pebble: NextPrefix unimplemented")
-}
-
-func (i *twoLevelCompactionIterator) Prev() *base.InternalKV {
-	panic("pebble: Prev unimplemented")
-}
-
-func (i *twoLevelCompactionIterator) skipForward(kv *base.InternalKV) *base.InternalKV {
-	if kv == nil {
-		for {
-			if key := i.topLevelIndex.Next(); key == nil {
-				break
-			}
-			result := i.loadIndex(+1)
-			if result != loadBlockOK {
-				if i.err != nil {
-					break
-				}
-				switch result {
-				case loadBlockFailed:
-					// We checked that i.index was at a valid entry, so
-					// loadBlockFailed could not have happened due to to i.index
-					// being exhausted, and must be due to an error.
-					panic("loadBlock should not have failed with no error")
-				case loadBlockIrrelevant:
-					panic("compactionIter should not be using block intervals for skipping")
-				default:
-					panic(fmt.Sprintf("unexpected case %d", result))
-				}
-			}
-			// result == loadBlockOK
-			if kv = i.singleLevelIterator.First(); kv != nil {
-				break
-			}
-		}
-	}
-
-	// We have an upper bound when the table is virtual.
-	if i.upper != nil && kv != nil {
-		cmp := i.cmp(kv.K.UserKey, i.upper)
-		if cmp > 0 || (!i.endKeyInclusive && cmp == 0) {
-			return nil
-		}
-	}
-
-	return kv
-}
diff --git a/sstable/reader_virtual.go b/sstable/reader_virtual.go
index d2582516..771cb6a3 100644
--- a/sstable/reader_virtual.go
+++ b/sstable/reader_virtual.go
@@ -101,11 +101,16 @@ func (v *VirtualReader) NewCompactionIter(
 		transforms, categoryAndQoS, statsCollector, rp, &v.vState, bufferPool)
 }
 
-// NewIterWithBlockPropertyFiltersAndContextEtc wraps
-// Reader.NewIterWithBlockPropertyFiltersAndContext. We assume that the passed
-// in [lower, upper) bounds will have at least some overlap with the virtual
-// sstable bounds. No overlap is not currently supported in the iterator.
-func (v *VirtualReader) NewIterWithBlockPropertyFiltersAndContextEtc(
+// NewPointIter returns an iterator for the point keys in the table.
+//
+// If transform.HideObsoletePoints is set, the callee assumes that filterer
+// already includes obsoleteKeyBlockPropertyFilter. The caller can satisfy this
+// contract by first calling TryAddBlockPropertyFilterForHideObsoletePoints.
+//
+// We assume that the [lower, upper) bounds (if specified) will have at least
+// some overlap with the virtual sstable bounds. No overlap is not currently
+// supported in the iterator.
+func (v *VirtualReader) NewPointIter(
 	ctx context.Context,
 	transforms IterTransforms,
 	lower, upper []byte,
@@ -116,7 +121,7 @@ func (v *VirtualReader) NewIterWithBlockPropertyFiltersAndContextEtc(
 	statsCollector *CategoryStatsCollector,
 	rp ReaderProvider,
 ) (Iterator, error) {
-	return v.reader.newIterWithBlockPropertyFiltersAndContext(
+	return v.reader.newPointIter(
 		ctx, transforms, lower, upper, filterer, filterBlockSizeLimit,
 		stats, categoryAndQoS, statsCollector, rp, &v.vState)
 }
diff --git a/sstable/value_block.go b/sstable/value_block.go
index f748bc22..308a7ce5 100644
--- a/sstable/value_block.go
+++ b/sstable/value_block.go
@@ -719,21 +719,28 @@ type ReaderProvider interface {
 	Close()
 }
 
-// TrivialReaderProvider implements ReaderProvider for a Reader that will
-// outlive the top-level iterator in the iterator tree.
-type TrivialReaderProvider struct {
-	*Reader
+// MakeTrivialReaderProvider creates a ReaderProvider which always returns the
+// given reader. It should be used when the Reader will outlive the iterator
+// tree.
+func MakeTrivialReaderProvider(r *Reader) ReaderProvider {
+	return (*trivialReaderProvider)(r)
 }
 
-var _ ReaderProvider = TrivialReaderProvider{}
+// trivialReaderProvider implements ReaderProvider for a Reader that will
+// outlive the top-level iterator in the iterator tree.
+//
+// Defining the type in this manner (as opposed to a struct) avoids allocation.
+type trivialReaderProvider Reader
+
+var _ ReaderProvider = (*trivialReaderProvider)(nil)
 
 // GetReader implements ReaderProvider.
-func (trp TrivialReaderProvider) GetReader(ctx context.Context) (*Reader, error) {
-	return trp.Reader, nil
+func (trp *trivialReaderProvider) GetReader(ctx context.Context) (*Reader, error) {
+	return (*Reader)(trp), nil
 }
 
 // Close implements ReaderProvider.
-func (trp TrivialReaderProvider) Close() {}
+func (trp *trivialReaderProvider) Close() {}
 
 // valueBlockReader is used to retrieve values in value
 // blocks. It is used when the sstable was written with
diff --git a/table_cache.go b/table_cache.go
index cd3cb225..031b5387 100644
--- a/table_cache.go
+++ b/table_cache.go
@@ -596,7 +596,7 @@ func (c *tableCacheShard) newPointIter(
 			transforms, categoryAndQoS, dbOpts.sstStatsCollector, rp,
 			internalOpts.bufferPool)
 	} else {
-		iter, err = cr.NewIterWithBlockPropertyFiltersAndContextEtc(
+		iter, err = cr.NewPointIter(
 			ctx, transforms, opts.GetLowerBound(), opts.GetUpperBound(), filterer, filterBlockSizeLimit,
 			internalOpts.stats, categoryAndQoS, dbOpts.sstStatsCollector, rp)
 	}


Test patch:
diff --git a/level_iter_test.go b/level_iter_test.go
index 27436e5c..9efed178 100644
--- a/level_iter_test.go
+++ b/level_iter_test.go
@@ -173,10 +173,10 @@ func (lt *levelIterTest) newIters(
 	transforms := file.IterTransforms()
 	var set iterSet
 	if kinds.Point() {
-		iter, err := lt.readers[file.FileNum].NewIterWithBlockPropertyFiltersAndContextEtc(
+		iter, err := lt.readers[file.FileNum].NewPointIter(
 			ctx, transforms,
 			opts.LowerBound, opts.UpperBound, nil, sstable.AlwaysUseFilterBlock, iio.stats, sstable.CategoryAndQoS{},
-			nil, sstable.TrivialReaderProvider{Reader: lt.readers[file.FileNum]})
+			nil, sstable.MakeTrivialReaderProvider(lt.readers[file.FileNum]))
 		if err != nil {
 			return iterSet{}, errors.CombineErrors(err, set.CloseAll())
 		}
diff --git a/merging_iter_test.go b/merging_iter_test.go
index 9cabc5e8..4bc721c9 100644
--- a/merging_iter_test.go
+++ b/merging_iter_test.go
@@ -175,10 +175,11 @@ func TestMergingIterCornerCases(t *testing.T) {
 				}
 			}
 			if kinds.Point() {
-				set.point, err = r.NewIterWithBlockPropertyFilters(
+				set.point, err = r.NewPointIter(
+					context.Background(),
 					sstable.NoTransforms,
 					opts.GetLowerBound(), opts.GetUpperBound(), nil, sstable.AlwaysUseFilterBlock, iio.stats,
-					sstable.CategoryAndQoS{}, nil, sstable.TrivialReaderProvider{Reader: r})
+					sstable.CategoryAndQoS{}, nil, sstable.MakeTrivialReaderProvider(r))
 				if err != nil {
 					return iterSet{}, errors.CombineErrors(err, set.CloseAll())
 				}
diff --git a/sstable/block_property_test.go b/sstable/block_property_test.go
index 9c959233..e2c4b646 100644
--- a/sstable/block_property_test.go
+++ b/sstable/block_property_test.go
@@ -971,9 +971,10 @@ func TestBlockProperties(t *testing.T) {
 			} else if !ok {
 				return "filter excludes entire table"
 			}
-			iter, err := r.NewIterWithBlockPropertyFilters(
+			iter, err := r.NewPointIter(
+				context.Background(),
 				NoTransforms, lower, upper, filterer, NeverUseFilterBlock, &stats,
-				CategoryAndQoS{}, nil, TrivialReaderProvider{Reader: r})
+				CategoryAndQoS{}, nil, MakeTrivialReaderProvider(r))
 			if err != nil {
 				return err.Error()
 			}
@@ -1053,9 +1054,10 @@ func TestBlockProperties_BoundLimited(t *testing.T) {
 			} else if !ok {
 				return "filter excludes entire table"
 			}
-			iter, err := r.NewIterWithBlockPropertyFilters(
+			iter, err := r.NewPointIter(
+				context.Background(),
 				NoTransforms, lower, upper, filterer, NeverUseFilterBlock, &stats,
-				CategoryAndQoS{}, nil, TrivialReaderProvider{Reader: r})
+				CategoryAndQoS{}, nil, MakeTrivialReaderProvider(r))
 			if err != nil {
 				return err.Error()
 			}
diff --git a/sstable/random_test.go b/sstable/random_test.go
index 387107dc..60596eb0 100644
--- a/sstable/random_test.go
+++ b/sstable/random_test.go
@@ -92,15 +92,16 @@ func runErrorInjectionTest(t *testing.T, seed int64) {
 		}, nil, nil)
 	}
 
-	// TOOD(jackson): NewIterWithBlockPropertyFilters returns an iterator over
-	// point keys only. Should we add variants of this test that run random
-	// operations on the range deletion and range key iterators?
+	// TOOD(jackson): NewPointIter returns an
+	// iterator over point keys only. Should we add variants of this test that run
+	// random operations on the range deletion and range key iterators?
 	var stats base.InternalIteratorStats
 	filterBlockSizeLimit := AlwaysUseFilterBlock
 	if rng.Intn(2) == 1 {
 		filterBlockSizeLimit = NeverUseFilterBlock
 	}
-	it, err := r.NewIterWithBlockPropertyFilters(
+	it, err := r.NewPointIter(
+		context.Background(),
 		NoTransforms,
 		nil /* lower TODO */, nil, /* upper TODO */
 		filterer,
@@ -108,7 +109,7 @@ func runErrorInjectionTest(t *testing.T, seed int64) {
 		&stats,
 		CategoryAndQoS{},
 		nil, /* CategoryStatsCollector */
-		TrivialReaderProvider{r},
+		MakeTrivialReaderProvider(r),
 	)
 	require.NoError(t, err)
 	defer it.Close()
diff --git a/sstable/reader_iter_test.go b/sstable/reader_iter_test.go
index f6fec501..598c6d31 100644
--- a/sstable/reader_iter_test.go
+++ b/sstable/reader_iter_test.go
@@ -63,7 +63,7 @@ func TestIteratorErrorOnInit(t *testing.T) {
 				&stats,
 				CategoryAndQoS{},
 				nil, /* statsCollector */
-				TrivialReaderProvider{Reader: r},
+				MakeTrivialReaderProvider(r),
 				&pool,
 			)
 			require.Error(t, err)
@@ -78,7 +78,7 @@ func TestIteratorErrorOnInit(t *testing.T) {
 				&stats,
 				CategoryAndQoS{},
 				nil, /* statsCollector */
-				TrivialReaderProvider{Reader: r},
+				MakeTrivialReaderProvider(r),
 				&pool,
 			)
 			require.Error(t, err)
diff --git a/sstable/reader_test.go b/sstable/reader_test.go
index 33702e61..6067c132 100644
--- a/sstable/reader_test.go
+++ b/sstable/reader_test.go
@@ -448,9 +448,9 @@ func runVirtualReaderTest(t *testing.T, path string, blockSize, indexBlockSize i
 			}
 
 			transforms := IterTransforms{SyntheticSuffix: syntheticSuffix}
-			iter, err := v.NewIterWithBlockPropertyFiltersAndContextEtc(
+			iter, err := v.NewPointIter(
 				context.Background(), transforms, lower, upper, filterer, NeverUseFilterBlock,
-				&stats, CategoryAndQoS{}, nil, TrivialReaderProvider{Reader: r})
+				&stats, CategoryAndQoS{}, nil, MakeTrivialReaderProvider(r))
 			if err != nil {
 				return err.Error()
 			}
@@ -827,7 +827,7 @@ func runTestReader(t *testing.T, o WriterOptions, dir string, r *Reader, printVa
 						return "table does not intersect BlockPropertyFilter"
 					}
 				}
-				iter, err := r.NewIterWithBlockPropertyFiltersAndContextEtc(
+				iter, err := r.NewPointIter(
 					context.Background(),
 					transforms,
 					nil, /* lower */
@@ -837,7 +837,7 @@ func runTestReader(t *testing.T, o WriterOptions, dir string, r *Reader, printVa
 					&stats,
 					CategoryAndQoS{},
 					nil,
-					TrivialReaderProvider{Reader: r},
+					MakeTrivialReaderProvider(r),
 				)
 				if err != nil {
 					return err.Error()
@@ -977,15 +977,15 @@ func TestCompactionIteratorSetupForCompaction(t *testing.T) {
 				var pool block.BufferPool
 				pool.Init(5)
 				citer, err := r.NewCompactionIter(
-					NoTransforms, CategoryAndQoS{}, nil, TrivialReaderProvider{Reader: r}, &pool)
+					NoTransforms, CategoryAndQoS{}, nil, MakeTrivialReaderProvider(r), &pool)
 				require.NoError(t, err)
 				switch i := citer.(type) {
-				case *compactionIterator:
+				case *singleLevelIterator:
 					require.True(t, objstorageprovider.TestingCheckMaxReadahead(i.dataRH))
 					// Each key has one version, so no value block, regardless of
 					// sstable version.
 					require.Nil(t, i.vbRH)
-				case *twoLevelCompactionIterator:
+				case *twoLevelIterator:
 					require.True(t, objstorageprovider.TestingCheckMaxReadahead(i.dataRH))
 					// Each key has one version, so no value block, regardless of
 					// sstable version.
@@ -1033,10 +1033,10 @@ func TestReadaheadSetupForV3TablesWithMultipleVersions(t *testing.T) {
 		var pool block.BufferPool
 		pool.Init(5)
 		citer, err := r.NewCompactionIter(
-			NoTransforms, CategoryAndQoS{}, nil, TrivialReaderProvider{Reader: r}, &pool)
+			NoTransforms, CategoryAndQoS{}, nil, MakeTrivialReaderProvider(r), &pool)
 		require.NoError(t, err)
 		defer citer.Close()
-		i := citer.(*compactionIterator)
+		i := citer.(*singleLevelIterator)
 		require.True(t, objstorageprovider.TestingCheckMaxReadahead(i.dataRH))
 		require.True(t, objstorageprovider.TestingCheckMaxReadahead(i.vbRH))
 	}
@@ -1323,12 +1323,12 @@ func TestRandomizedPrefixSuffixRewriter(t *testing.T) {
 		}
 		eReader, err := newReader(f, opts)
 		require.NoError(t, err)
-		iter, err := eReader.newIterWithBlockPropertyFiltersAndContext(
+		iter, err := eReader.newPointIter(
 			context.Background(),
 			block.IterTransforms{SyntheticSuffix: syntheticSuffix, SyntheticPrefix: syntheticPrefix},
 			nil, nil, nil,
 			AlwaysUseFilterBlock, nil, CategoryAndQoS{}, nil,
-			TrivialReaderProvider{Reader: eReader}, &virtualState{
+			MakeTrivialReaderProvider(eReader), &virtualState{
 				lower: base.MakeInternalKey([]byte("_"), base.SeqNumMax, base.InternalKeyKindSet),
 				upper: base.MakeRangeDeleteSentinelKey([]byte("~~~~~~~~~~~~~~~~")),
 			})
@@ -2448,10 +2448,10 @@ func BenchmarkIteratorScanObsolete(b *testing.B) {
 									}
 								}
 								transforms := IterTransforms{HideObsoletePoints: hideObsoletePoints}
-								iter, err := r.NewIterWithBlockPropertyFiltersAndContextEtc(
+								iter, err := r.NewPointIter(
 									context.Background(), transforms, nil, nil, filterer,
 									AlwaysUseFilterBlock, nil, CategoryAndQoS{}, nil,
-									TrivialReaderProvider{Reader: r})
+									MakeTrivialReaderProvider(r))
 								require.NoError(b, err)
 								b.ResetTimer()
 								for i := 0; i < b.N; i++ {
diff --git a/sstable/testdata/virtual_reader_iter b/sstable/testdata/virtual_reader_iter
index 3c09dd11..1c23a66b 100644
--- a/sstable/testdata/virtual_reader_iter
+++ b/sstable/testdata/virtual_reader_iter
@@ -55,9 +55,8 @@ scan-range-key
 ----
 a-d:{(#11,RANGEKEYSET,@10,foo)}
 
-# Test iterators with various bounds, and various operations. This calls
-# VirtualReader.NewIterWithBlockPropertyFilters and performs various operations
-# on those.
+# Test iterators with various bounds, and various operations. This creates a new
+# iterator using the virtual reader and performs various operations on those.
 build
 a.SET.1:a
 b.SET.2:b