From f4eabe83ef8f79c97830bd3a92dd4a2e052cf676 Mon Sep 17 00:00:00 2001 From: gammazero <11790789+gammazero@users.noreply.github.com> Date: Wed, 24 Sep 2025 12:48:17 -1000 Subject: [PATCH] Remove iterator interface See #230 for context. Closes #230 --- engine/engine.go | 4 -- interface.go | 16 ----- store/dhstore/dhstore.go | 4 -- store/memory/memory.go | 32 ---------- store/pebble/pebble.go | 129 +++++---------------------------------- store/test/tests.go | 47 -------------- 6 files changed, 14 insertions(+), 218 deletions(-) diff --git a/engine/engine.go b/engine/engine.go index 01c397a..0b177e8 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -208,10 +208,6 @@ func (e *Engine) Close() error { return e.valueStore.Close() } -func (e *Engine) Iter() (indexer.Iterator, error) { - return e.valueStore.Iter() -} - func (e *Engine) updateCacheStats() { st := e.resultCache.Stats() var prevStats *cache.Stats diff --git a/interface.go b/interface.go index ad4f882..dc580ea 100644 --- a/interface.go +++ b/interface.go @@ -47,27 +47,11 @@ type Interface interface { // Close gracefully closes the store flushing all pending data from memory, Close() error - // Iter creates a new value store iterator. - Iter() (Iterator, error) - // Stats returns statistical information about the indexed values. // If unsupported by the backing store, ErrStatsNotSupported is returned. Stats() (*Stats, error) } -// Iterator iterates multihashes and values in the value store. Any write -// operation invalidates the iterator. -type Iterator interface { - // Next returns the next multihash and the value it indexer. Returns io.EOF - // when finished iterating. - Next() (multihash.Multihash, []Value, error) - - // Close closes the iterator releasing any resources that may be occupied by it. - // The iterator will no longer be usable after a call to this function and is - // discarded. - Close() error -} - // Stats provides statistics about the indexed values. type Stats struct { // MultihashCount is the number of unique multihashes indexed. diff --git a/store/dhstore/dhstore.go b/store/dhstore/dhstore.go index 5b5a702..9b5fa12 100644 --- a/store/dhstore/dhstore.go +++ b/store/dhstore/dhstore.go @@ -282,10 +282,6 @@ func (s *dhStore) Flush() error { return nil } func (s *dhStore) Close() error { return nil } -func (s *dhStore) Iter() (indexer.Iterator, error) { - return nil, ErrNotSupported -} - func (s *dhStore) Stats() (*indexer.Stats, error) { return nil, nil } diff --git a/store/memory/memory.go b/store/memory/memory.go index c23601b..251b2a1 100644 --- a/store/memory/memory.go +++ b/store/memory/memory.go @@ -12,8 +12,6 @@ import ( "bytes" "context" "errors" - "io" - "iter" "strings" "sync" @@ -31,11 +29,6 @@ type memoryStore struct { mutex sync.Mutex } -type memoryIter struct { - iterNext func() (string, []*indexer.Value, bool) - iterStop func() -} - var _ indexer.Interface = (*memoryStore)(nil) // New creates a new indexer.Interface implemented by a radixtree-based value @@ -188,14 +181,6 @@ func (s *memoryStore) Flush() error { return nil } func (s *memoryStore) Close() error { return nil } -func (s *memoryStore) Iter() (indexer.Iterator, error) { - next, stop := iter.Pull2[string, []*indexer.Value](s.rtree.Iter()) - return &memoryIter{ - iterNext: next, - iterStop: stop, - }, nil -} - func (s *memoryStore) Stats() (*indexer.Stats, error) { var count uint64 s.mutex.Lock() @@ -207,23 +192,6 @@ func (s *memoryStore) Stats() (*indexer.Stats, error) { }, nil } -func (it *memoryIter) Next() (multihash.Multihash, []indexer.Value, error) { - key, vals, ok := it.iterNext() - if !ok { - return nil, nil, io.EOF - } - values := make([]indexer.Value, len(vals)) - for i, v := range vals { - values[i] = *v - } - return multihash.Multihash(key), values, nil -} - -func (it *memoryIter) Close() error { - it.iterStop() - return nil -} - func (s *memoryStore) get(k string) ([]*indexer.Value, bool) { // Search current cache return s.rtree.Get(k) diff --git a/store/pebble/pebble.go b/store/pebble/pebble.go index bf2d46a..1aa6b47 100644 --- a/store/pebble/pebble.go +++ b/store/pebble/pebble.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "errors" - "io" "slices" "time" @@ -28,34 +27,22 @@ var ( log = logging.Logger("store/pebble") _ indexer.Interface = (*store)(nil) - _ indexer.Iterator = (*iterator)(nil) ) -type ( - store struct { - db *pebble.DB - // Only support binary format since in pebble we need the capability to merge keys and - // there is little reason for store values in any format other than binary for performance - // characteristics. - // Note, pebble is using a zero-copy variation of marshaller to allow optimizations in - // cases where the value need not to be copied. The root level binary codec copies on - // unmarshal every time. - vcodec *codec - p *pool - closed bool - comparer *pebble.Comparer - metricsCancel context.CancelFunc - } - iterator struct { - closed bool - snapshot *pebble.Snapshot - it *pebble.Iterator - vcodec *codec - p *pool - start *key - end *key - } -) +type store struct { + db *pebble.DB + // Only support binary format since in pebble we need the capability to merge keys and + // there is little reason for store values in any format other than binary for performance + // characteristics. + // Note, pebble is using a zero-copy variation of marshaller to allow optimizations in + // cases where the value need not to be copied. The root level binary codec copies on + // unmarshal every time. + vcodec *codec + p *pool + closed bool + comparer *pebble.Comparer + metricsCancel context.CancelFunc +} // New instantiates a new instance of a store backed by Pebble. // Note that any Merger value specified in the given options will be overridden. @@ -316,91 +303,3 @@ func (s *store) Stats() (*indexer.Stats, error) { } return &stats, nil } - -func (s *store) Iter() (indexer.Iterator, error) { - keygen := s.p.leaseBlake3Keyer() - start, end, err := keygen.multihashesKeyRange() - if err != nil { - _ = keygen.Close() - return nil, err - } - _ = keygen.Close() - snapshot := s.db.NewSnapshot() - iter, err := snapshot.NewIter(&pebble.IterOptions{ - LowerBound: start.buf, - UpperBound: end.buf, - }) - if err != nil { - return nil, err - } - iter.First() - return &iterator{ - snapshot: snapshot, - it: iter, - vcodec: s.vcodec, - start: start, - end: end, - p: s.p, - }, nil -} - -func (i *iterator) Next() (multihash.Multihash, []indexer.Value, error) { - switch { - case i.it.Error() != nil: - return nil, nil, i.it.Error() - case !i.it.Valid(): - return nil, nil, io.EOF - } - keygen := i.p.leaseBlake3Keyer() - mhk := i.p.leaseKey() - mhk.append(i.it.Key()...) - mh, err := keygen.keyToMultihash(mhk) - _ = mhk.Close() - _ = keygen.Close() - if err != nil { - return nil, nil, err - } - - // We don't need to copy the value since it is only used for fetching the - // indexer.Values, and not returned to the caller. - vks, err := i.vcodec.unmarshalValueKeys(i.it.Value()) - if err != nil { - return nil, nil, err - } - defer vks.Close() - vs := make([]indexer.Value, len(vks.keys)) - for j, vk := range vks.keys { - bv, c, err := i.snapshot.Get(vk.buf) - if err != nil { - return nil, nil, err - } - - v, err := i.vcodec.unmarshalValue(bv) - _ = c.Close() - if err != nil { - return nil, nil, err - } - vs[j] = *v - } - i.it.Next() - return mh, vs, err -} - -func (i *iterator) Close() error { - _ = i.start.Close() - _ = i.end.Close() - // Check if closed already and do not re-call, since pebble - // panics if snapshot is closed more than once. - if i.closed { - return nil - } - serr := i.snapshot.Close() - ierr := i.it.Close() - i.closed = true - // Prioritise returning the iterator closure error. Because, that error - // is more likely to be meaningful to the caller. - if ierr != nil { - return ierr - } - return serr -} diff --git a/store/test/tests.go b/store/test/tests.go index 9e82245..8edd480 100644 --- a/store/test/tests.go +++ b/store/test/tests.go @@ -2,7 +2,6 @@ package test import ( "context" - "io" "sync" "testing" @@ -131,52 +130,6 @@ func E2ETest(t *testing.T, s indexer.Interface) { t.Fatal("Got wrong value for single multihash") } - // Iterate values - t.Log("Iterating values") - var indexCount int - seen := make(map[string]struct{}) - iter, err := s.Iter() - if err != nil { - t.Fatal(err) - } - defer iter.Close() - - for { - m, _, err := iter.Next() - if err != nil { - if err == io.EOF { - break - } - t.Fatalf("Iteration error: %s", err) - } - - mb58 := m.B58String() - t.Logf("Visited: %s", mb58) - _, already := seen[mb58] - if already { - t.Errorf("Error: multihash already seen: %q", mb58) - } else { - seen[mb58] = struct{}{} - } - indexCount++ - } - t.Logf("Visited %d multihashes", indexCount) - if indexCount != len(batch)+1 { - t.Errorf("Wrong iteration count: expected %d, got %d", len(batch)+1, indexCount) - } - for i := range batch { - b58 := batch[i].B58String() - _, ok := seen[b58] - if !ok { - t.Fatalf("Did not iterate multihash %s", b58) - } - } - - _, _, err = iter.Next() - if err != io.EOF { - t.Fatal("caling iter.Next() after iteration finished should yield same result") - } - // Get a key that is not set t.Log("Get non-existing key") _, found, err = s.Get(noadd)