Iterator utilities and simplfications#3513
Conversation
PR SummaryMedium Risk Overview It adds reusable FlatKV The thin Pebble Reviewed by Cursor Bugbot for commit 9948bbb. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3513 +/- ##
==========================================
- Coverage 59.04% 58.66% -0.38%
==========================================
Files 2199 2164 -35
Lines 182096 178455 -3641
==========================================
- Hits 107510 104690 -2820
+ Misses 64935 64351 -584
+ Partials 9651 9414 -237
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| }, keys) | ||
| } | ||
|
|
||
| func TestMergingIterator_DuplicateKeys(t *testing.T) { |
There was a problem hiding this comment.
can we have a comment in the func NewMergingIterator describe the intentional duplicate keys behavior
There was a problem hiding this comment.
You make a very good point.
Although not broken for the flatKV raw iterator, the current merge behavior in this branch was actually incorrect. It currently emits duplicate keys if they show up in multiple child iterators, but it really should be de-duping. This will be necessary for future changes that come after this PR.
I've fixed this bug and documented the behavior.
|
|
||
| // NewMappingIterator returns an iterator that emits remapped key/value pairs | ||
| // from parent, skipping pairs for which remapper returns skip=true. | ||
| func NewMappingIterator(parent dbm.Iterator, remapper IteratorRemapper) *mappingIterator { |
There was a problem hiding this comment.
do we want to return *mappingIterator or dbm.Iterator as other constructors
There was a problem hiding this comment.
Changed return type to dbm.Iterator
| } | ||
| func (f *failingEVMStore) Has(string, []byte) bool { return false } | ||
| func (f *failingEVMStore) RawGlobalIterator() flatkv.Iterator { return nil } | ||
| func (f *failingEVMStore) RawGlobalIterator() dbm.Iterator { return nil } |
There was a problem hiding this comment.
right now RawGlobalIterator() dbm.Iterator swallows construction errors into an invalidIterator whose Error() reports the cause. This works only if every caller remembers to check Error() after the loop, a missed check makes a failed iterator open indistinguishable from an empty store.
how about we change the signature to RawGlobalIterator() (dbm.Iterator, error). would also let us drop iterators.NewInvalidIterator since looks it has no other production user
There was a problem hiding this comment.
Good suggestion, done.
I really dislike the dbm.Iterator API. It would be MUCH better to return errors when they actually happen, instead of hoping that people remember to call the Error() method. I want to keep the scope of this PR small. But in the future, my plan is to change the API to something more sensible. That should be a lot easier in the future, since we're refactoring things so that everything uses the same iterator interface.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d673ffd. Configure here.

Describe your changes and provide context
This PR does prep work for flatKV iteration and iteration over modules during migration.
Interface simplfiication
We had a large number of iterator interfaces defined in different packages. Each iterator interface did almost the same thing, but was slightly different. Since we need to construct iterators that walk over the data in multiple packages, this was making things overlay complex.
I simplified things by selecting the the iterator interface used by
composite.Storeand making that interface the one that all DBs implement.Iterator utilities
In order handle iteration on flatKV and on stores in the middle of migration, we need ways to combine and modify iterators. Now that all of the iterators have the same type, we can write these utilities generically and reuse them, as opposed to building a dozen structs that do almost the same thing. There are two new utility structs introduced by this PR:
(key, value)pairs into different(key, value)pairsIn this PR, I used those iterators to re-implement flatKV's exporter. In the next PR, I will also use these utilities to implement cross-DB and mid-migration iteration.
Testing performed to validate your change
unit tests