perf(frag-reuse): build the index-open path on RowAddrRemap (release-9, for redap 0.16) - #40
Open
rerun-rmack wants to merge 3 commits into
Open
perf(frag-reuse): build the index-open path on RowAddrRemap (release-9, for redap 0.16)#40rerun-rmack wants to merge 3 commits into
rerun-rmack wants to merge 3 commits into
Conversation
Forward-port of #38 to release-9. `RowAddrRemap` is already here (upstream lance-format#7237), but `open_frag_reuse_index` was never converted to it, on this branch or on upstream main and v10.0. It built a `HashMap<u64, Option<u64>>` per reuse version with one entry per remapped row, on every index open, cached, so readers pay it. Measured on a production payload: 676,592,102 entries, 88 MB on disk becoming 26.8 GB resident and 40.5 GB peak, 144 s to build. It OOMs a 60 GiB pod. Against the same payload, `RowAddrRemap::compact` builds in 0.1 s and 0.27 GB peak, and agrees with the map on all 676,595,694 addresses probed. The only differences are 3,592 offsets past a fragment's `physical_rows`, where the map says absent and compact says deleted; that is compact's documented behaviour and those addresses are not rows. This is the residual delta for redap 0.16: everything in #38 except the backport, which this branch already has. Three fork-local additions to the upstream module, marked as such above the test module: `Debug` and `DeepSizeOf` for `RowAddrRemap`, plus the `num_groups`/`num_fragments` accessors those use. `FragReuseIndex` needs both; upstream does not yet, because it still holds hashmaps there. `remap_column_index` composed a map by enumerating keys, and the compact form deliberately cannot enumerate: it stores per-fragment bitmaps and treats any unlisted offset in a rewritten fragment as deleted, so its key set is not finite. That path now rebuilds the per-row maps from the details. Still O(rows), but only tests reach it in-tree and no redap caller does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Forward-port of #39 to release-9. The fragment-reuse payload records which row addresses a compaction moved, as a serialized RoaringTreemap. Compaction rewrites long contiguous spans, but nothing calls `RoaringTreemap::optimize()`, so those spans are written as dense bitmap containers at one bit per row. On a real payload that is 88 MB for 676M addresses, essentially all bitmap containers: 676M bits is 84.6 MB. This matters more than it looks. `RowAddrRemap::Compact` keeps those same bitmaps resident, so its 88 MB in-memory footprint is the same containers. Run-optimizing shrinks the stored object, the deserialize on every index open, and the compact remap's own memory. No format change: run containers are already produced today by the binary-copy path via `insert_range`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lockfile only, no manifest change; both are patch bumps within the same minor. h2 0.4.15 -> 0.4.16 RUSTSEC-2026-0258 rkyv 0.8.16 -> 0.8.18 RUSTSEC-2026-0233, -0234, -0235 Pre-existing on the branch and unrelated to the change this PR carries. The last green run here was 2026-07-29, before any of the four reached the advisory database: the rkyv entries landed 2026-08-04 and the h2 one 2026-08-18, which is why identical inputs started failing. Neither is urgent on its own merits. rkyv is in the lockfile but not the resolved graph (`cargo tree -i rkyv` matches nothing); the only thing that pulls it is `lindera-dictionary`, behind the optional `tokenizer-lindera` feature, so we never compile it. The h2 advisory is a denial of service driven by a peer sending empty DATA frames, and we are an HTTP/2 client to AWS, so the peer is S3 or DynamoDB behind TLS. Bumping anyway because the fix is free and leaving CI red hides the next one. `cargo deny check` now reports advisories ok, bans ok, licenses ok, sources ok. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@rerun-rmack, hello, please know I somehow got tagged and CC'd on this PR. I received an e-mail about it. I just wanted to let you know that somehow my rmack Github ID was tagged. Cheers From Adam Cimarosti to rerun-io/lance and CC'd rmack (me). |
Author
Hah, thank's for letting me know. |
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
2 participants
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.
Release-9 counterpart of #38 and #39, carrying the same behaviour forward for redap 0.16.
Why there is still work to do here
RowAddrRemapalready exists on this branch, from upstream lance-format#7237. But upstream applied it to the scalar-index remap consumers and never convertedopen_frag_reuse_index, which is the path that OOMs us. That is still true on this branch, onupstream/main, and onrelease/v10.0.So moving to release-9 does not fix the OOM on its own. This is the residual delta.
Commits
Measured, on a real 88 MB production payload
compactaccepted all 428 groups, so neither ofGroupRemap::new's hard-error conditions (ascending new-fragment ids, rewritten-row count matching) is violated by data we have already committed. That was the risk worth checking, since on the read path a rejection would fail every query on the dataset.Verified against the map over 676,595,694 addresses: zero unexpected divergences. The 3,592 differences are all one predicted class, offsets past a fragment's
physical_rowswhere the map says absent and compact says deleted. Those are not rows, and it is compact's documented behaviour.Fork-local additions
Marked as such above the test module in
row_addr_remap.rs, so the delta against upstream stays visible:DebugandDeepSizeOfforRowAddrRemap, plusnum_groups/num_fragments.FragReuseIndexneeds both; upstream does not yet because it still holds hashmaps there. They will need these when they convert the open path.remap_column_indexcomposed a map by enumerating keys, and the compact form deliberately cannot enumerate. That path now rebuilds per-row maps from the details: O(rows) again, but only tests reach it in-tree and no redap caller does.Why #39 matters more here than it looked
Compactkeeps the roaring bitmaps resident, so its 88 MB in-memory footprint is those same dense containers. Run-optimizing shrinks the stored object, the deserialize on every index open, and the compact remap's own memory.Testing
lance-corerow_addr_remap 5,lance-table127,lancefrag_reuse 7,optimize::111. Clippy and fmt clean.Unlike #38 and #39, this branch gets real CI:
rust.ymlon release-9 matchesrelease-*, which release-8.0.0's does not.