From a6580b190400fdecfe776e45682e97bc410583d9 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Mon, 29 Jul 2024 19:12:37 -0700 Subject: [PATCH] sstable: remove compaction iterator wrappers The `compactionIterator` and `twoLevelCompactionIterator` wrappers no longer serve any purpose. --- sstable/reader.go | 14 ++--- sstable/reader_iter.go | 97 ---------------------------------- sstable/reader_iter_two_lvl.go | 94 -------------------------------- sstable/reader_test.go | 6 +-- 4 files changed, 11 insertions(+), 200 deletions(-) diff --git a/sstable/reader.go b/sstable/reader.go index 7c18792d0f..2c3a0f07cc 100644 --- a/sstable/reader.go +++ b/sstable/reader.go @@ -222,10 +222,12 @@ 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 NewIterWithBlockPropertyFiltersAndContextEtc 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. @@ -268,7 +270,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 */ @@ -278,7 +280,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_iter.go b/sstable/reader_iter.go index 52cae69a59..d33974485a 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 1ebf9b68bd..8ad7419fbe 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_test.go b/sstable/reader_test.go index 2f28c01423..3f771b6afc 100644 --- a/sstable/reader_test.go +++ b/sstable/reader_test.go @@ -980,12 +980,12 @@ func TestCompactionIteratorSetupForCompaction(t *testing.T) { 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. @@ -1036,7 +1036,7 @@ func TestReadaheadSetupForV3TablesWithMultipleVersions(t *testing.T) { 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)) }