iterv2: implement NextPrefix in mergingIterV2#6005
iterv2: implement NextPrefix in mergingIterV2#6005RaduBerinde merged 2 commits intocockroachdb:masterfrom
Conversation
Previously, levelIterV2.NextPrefix panicked with "not implemented". This commit implements it by delegating to the per-file InterleavingIter, falling back to a level-level SeekGE when the file iterator exhausts. The level_iter_v2 randomized test no longer suppresses TestOpNextPrefix. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
sumeerbhola
left a comment
There was a problem hiding this comment.
@sumeerbhola reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on RaduBerinde).
merging_iter_v2.go line 815 at r2 (raw file):
default: // Boundary key, exhausted, or already >= succKey: SeekGE with // TrySeekUsingNext.
Do we not bother distinguishing between these cases because for the exhausted or already >= succKey case we are relying on the SeekGE not doing much?
It still seems worthwhile avoiding any calls: we've desired NextPrefix to be almost as fast as Next.
merging_iter_v2.go line 821 at r2 (raw file):
return nil } }
I am trying to think through the approach difference between what is happening here and in mergingIter. At a very high-level I think mergingIter.NextPrefix manages to do two things:
- Incrementally maintain the heap, versus doing an initHeap.
- Observe that the heap top is >= succKey to avoid positioning any other levels.
In the common case we may be stepping through multiple prefixes in L6, and the slab boundary is far away. Could we use the fact that NextPrefix is called when not at a slab boundary to NextPrefix the heap top, and if it is still at the top and not at the slab boundary we have nothing else to do. Even if the top level changed, but it is not a slab boundary, isn't it sufficient to check that it is >= succKey. The mergingIter does something more involved with levelsPositioned but it may not be necessary in this slab world.
f7d8261 to
f59792b
Compare
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
Previously, mergingIterV2.NextPrefix fell back to a full SeekGE on the merging iter. This commit implements it directly, driving the per-level NextPrefix fast path where applicable. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
f59792b to
13d6dbb
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
@RaduBerinde made 2 comments.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on sumeerbhola).
merging_iter_v2.go line 815 at r2 (raw file):
Previously, sumeerbhola wrote…
Do we not bother distinguishing between these cases because for the exhausted or already >= succKey case we are relying on the SeekGE not doing much?
It still seems worthwhile avoiding any calls: we've desiredNextPrefixto be almost as fast asNext.
Good point, fixed.
merging_iter_v2.go line 821 at r2 (raw file):
Previously, sumeerbhola wrote…
I am trying to think through the approach difference between what is happening here and in
mergingIter. At a very high-level I thinkmergingIter.NextPrefixmanages to do two things:
- Incrementally maintain the heap, versus doing an initHeap.
- Observe that the heap top is >= succKey to avoid positioning any other levels.
In the common case we may be stepping through multiple prefixes in L6, and the slab boundary is far away. Could we use the fact that
NextPrefixis called when not at a slab boundary to NextPrefix the heap top, and if it is still at the top and not at the slab boundary we have nothing else to do. Even if the top level changed, but it is not a slab boundary, isn't it sufficient to check that it is >= succKey. ThemergingIterdoes something more involved withlevelsPositionedbut it may not be necessary in this slab world.
I added a TODO and will tackle it separately. It's not trivial because if we discover a rangedel when we Next() that level, or the level goes too far, we do need to rebuild the slab. There is a similar TODO in SeekGE(TrySeekUsingNext); both fast paths will likely look similar.
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
sumeerbhola
left a comment
There was a problem hiding this comment.
@sumeerbhola made 1 comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on RaduBerinde).
|
TFTR! |
iterv2: implement NextPrefix in levelIterV2
Previously, levelIterV2.NextPrefix panicked with "not implemented".
This commit implements it by delegating to the per-file
InterleavingIter, falling back to a level-level SeekGE when the file
iterator exhausts.
The level_iter_v2 randomized test no longer suppresses
TestOpNextPrefix.
Co-Authored-By: roachdev-claude roachdev-claude-bot@cockroachlabs.com
iterv2: implement NextPrefix in mergingIterV2
Previously, mergingIterV2.NextPrefix fell back to a full SeekGE on
the merging iter. This commit implements it directly, driving the
per-level NextPrefix fast path where applicable.
Co-Authored-By: roachdev-claude roachdev-claude-bot@cockroachlabs.com