perf(rowids): probe the fragments instead of merging every row id - #8624
Open
pengw0048 wants to merge 9 commits into
Open
perf(rowids): probe the fragments instead of merging every row id#8624pengw0048 wants to merge 9 commits into
pengw0048 wants to merge 9 commits into
Conversation
…e index RowIdIndex::new walked every row id of every fragment whose sequence is not a plain Range, and get_row_id_index builds the index for the whole dataset, so one point lookup paid for the whole table. Keep one entry per fragment with per-segment bounds instead, and resolve on lookup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Peng Wang <peng.wang@lumalabs.ai>
pengw0048
marked this pull request as draft
August 18, 2026 22:29
…eement Build a row-id to position map for U64Segment::Array, the one encoding whose position() walks the segment. Resolve get_many through get so the two APIs return the same address for a row id that two fragments claim. Charge the sequences, deletion vectors and position maps the index retains through its Arcs, so a cache that weighs the entry bounds what the entry keeps alive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Peng Wang <peng.wang@lumalabs.ai>
…rlap new() checks again that no row id is live in two fragments, so the integrity boundary stays where it was. Two fragments can only claim an id where their row id ranges intersect, so the check walks the intersecting pairs and only the ids inside each intersection. Drops verify(), which no caller reached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Peng Wang <peng.wang@lumalabs.ai>
A prefix maximum cannot stop a backward walk once one fragment covers a wide range, so a miss walked every fragment whose start sits below the id: 4.46 us per miss at 16,384 fragments against 2 ns before this branch. Descend a max-end tree instead, which holds at 233 ns as the fragment count grows. Fragments whose ranges interleave produce a quadratic number of intersecting pairs, and walking their ids cost 1.61 s at 2,048 fragments. Estimate that work first and skip it above a budget; validate() runs it on demand. Fill the array position table on first use so a table nobody looks up costs nothing, which returns deep_size_of to what it was before the table existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Peng Wang <peng.wang@lumalabs.ai>
pengw0048
marked this pull request as ready for review
August 19, 2026 17:01
…cheap Keep the merged map for the shapes it serves well, and answer from a per-fragment probe otherwise. `new` reads each segment's bounds instead of every row id, and builds the merged map when that build is cheap or once probes have paid for it.
pengw0048
marked this pull request as draft
August 19, 2026 20:14
pengw0048
marked this pull request as ready for review
August 20, 2026 14:45
Three review findings, all of them about the lazy build: - The representation is now chosen by `new` and never changes, so a cached entry cannot grow after its weight is fixed. The lazy upgrade was solving a case that does not exist: wherever the merged map reads faster it is also the one `new` builds, and where it does not, probing wins on both build and lookup. - An unsorted `Array` segment gets a sorted lookup table, built with the entry and charged with it, so a probe binary-searches instead of scanning. `build_chunk_from_pairs` sorts its pairs for the same reason, which also makes a merged chunk searchable. - A probe visits every candidate instead of stopping at the first hit, so a row id live in two fragments resolves the same way through `get` and `get_many`, and trips an assertion in a debug build.
…agments The probe path visits every candidate fragment, so the duplicate check is free; it now returns the same corrupt-index error the merged build raises, instead of a debug assertion. get and get_many are fallible and every production caller propagates the error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
This revision documents the fallible lookup contract with explicit error sections and leaves the verified probe behavior unchanged: both lookup APIs still fail closed when a requested stable ID is live in multiple fragments.
Please mark this PR with the breaking-change label.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
RowIdIndex::newreads every row id of every fragment whose sequence is not aplain
Range.decompose_sequencehas a constant-time path forU64Segment::Rangewithout deletions; every other encoding, and every fragmentthat carries a deletion file, materializes one
(row_id, address)pair per row,sorts them, and re-encodes two segments.
get_row_id_indexbuilds the index forthe whole dataset, so a lookup of one row id pays for the whole table.
A delete puts a table on that path and keeps it there. While the deletion files
exist, the build checks the deletion vector row by row. After a compaction
materializes those deletions, the ids inside each fragment are no longer
contiguous, so the build decodes and re-encodes every row id instead.
The cost repeats: the index weighs about as much as the sequences it copies, so
on a large table it does not fit
DEFAULT_METADATA_CACHE_SIZE, and the nextlookup builds it again.
Change
newpicks one of two representations and never changes it.Range-only sequence decomposes in constant time, and a small table is cheapto read whatever its encoding.
Arc<RowIdSequence>,the deletion vector, and each segment's row id range plus its physical start
offset.
newreads those bounds instead of every row id. A lookup binary-searches the fragments by their lowest row id, descends a max-
endheap so afragment out of reach of the id costs nothing, and asks the covering segment
for
position.newprobes only when both hold: the merged build would read more thanMERGE_ROWS_BUDGETrow ids, and no more thanMAX_PROBE_DEPTHfragments coverany one id. So wherever the merged map reads faster, it is also the one
newbuilds.
An unsorted
U64Segment::Arraygets a sorted lookup table, built with the entryand charged with it, because its own
positionscans.build_chunk_from_pairssorts its pairs for the same reason, which makes a merged chunk searchable too.
Measurements
Release build, 96-core Linux x86-64.
buildisRowIdIndex::new; the lookupcolumns are 100,000 random live ids.
getget_manyArray)Row 1 is the shape a compacted table has, and the merged map is slower to build
and slower to read there: merging the overlap produces one wide sparse segment
whose
positioncounts bits up to the offset.Row 2 is where the merged map earns its build, and
newbuilds it, so bothcolumns match.
Row 4 is the finding about unsorted arrays, in both representations.
On a real table
A dataset of 15,431,187,521 rows in 17,601 fragments, holding 3.36 GB of row id
sequences. Half of the segments are
RangeWithBitmaporRangeWithHoles, whichis what a compaction leaves behind once it materializes a long run of deletes.
46 fragments cover the most crowded row id.
_take_rowsof one column, in afresh process:
(Measured on the earlier revision of this PR; the representation it picks for
this table is the same one, and #8642 takes the 1.10 s to 0.61 s.)
The integrity boundary
newno longer materializes ids on the probe path, so it cannot compare id setsthe way
merge_overlapping_chunksdoes. The check moved to the lookup, where itis free: a probe already visits every candidate fragment rather than stopping at
the first hit, and a second live match returns the same corrupt-index error the
merged build raises.
getandget_manyare fallible for this, and everyproduction caller propagates the error.
validate_stable_row_idsremains thecomplete offline check.
Checking at construction instead is possible but not cheap on the shape this PR
is for: proving overlapping id sets disjoint without reading them means
intersecting the sequences bit by bit, on the order of 10^10 word operations at
depth 46 over that table's id space, paid again on every process that opens the
dataset. The fallible lookup keeps the boundary without that cost.
Closes #8621