perf(frag-reuse): build the index-open path on RowAddrRemap instead of a per-row HashMap - #38
Closed
rerun-rmack wants to merge 3 commits into
Closed
perf(frag-reuse): build the index-open path on RowAddrRemap instead of a per-row HashMap#38rerun-rmack wants to merge 3 commits into
rerun-rmack wants to merge 3 commits into
Conversation
Member
|
copy pasting just as extra inputs: |
rerun-rmack
force-pushed
the
rmack/fri-runs-map
branch
from
August 18, 2026 12:13
8cf1928 to
571ac04
Compare
rerun-rmack
changed the title
rerun-rmack
marked this pull request as ready for review
August 18, 2026 12:47
Verbatim copy of `rust/lance-core/src/utils/row_addr_remap.rs` as it exists on `release-9.0.0`, from upstream lance-format#7237 ("introduce RowAddrRemap structure to avoid remap OOM caused by HashMap", merged 2026-07-03), plus the module registration. No edits, so the file diffs clean against release-9. Nothing uses it yet; the next commit does. Upstream applied this to the scalar-index remap consumers but left the fragment-reuse index-open path on `HashMap<u64, Option<u64>>`, which is the path that OOMs us. That is still true on main and every release branch through v10.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`open_frag_reuse_index` built a `HashMap<u64, Option<u64>>` per reuse version with one entry per remapped row. It runs on every index open and the result is 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. That OOMs a 60 GiB pod. Builds `RowAddrRemap::Compact` from the same `FragReuseGroup` fields instead, which is O(#fragments). `GroupInput` takes exactly what a group already holds: the deserialized `changed_row_addrs`, the old fragment ids, and the new fragments as (id, physical_rows). `FragReuseIndex::new` still accepts maps and stores them as `RowAddrRemap::Direct`, so existing callers keep working unchanged. Three fork-local additions to the backported module, marked as such at the bottom of the file so the diff against release-9 stays obvious: `Debug` and `DeepSizeOf` for `RowAddrRemap` (it is stored in `FragReuseIndex`, which needs both, and upstream does not yet because it still holds hashmaps there), plus the `num_groups`/`num_fragments` accessors those impls use. One consumer had to change shape. `remap_column_index` composed a map across versions 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 it is reached only by tests in-tree and by no caller in redap, whereas the open path above is on every query. 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. Matches the same bump on the release-9 branch, where cargo-deny actually runs in CI; release-8.0.0's `rust.yml` triggers on `main` and `release/**`, which our branch name does not match, so nothing here is enforced. 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. This does not make `cargo deny` green on release-8. Three unrelated advisories remain, all of which release-9 already handles: RUSTSEC-2026-0194/0195 (quick-xml) are on release-9's deny.toml ignore list but not this branch's, and RUSTSEC-2026-0204 (crossbeam-epoch 0.9.18) is fixed there by 0.9.20. Left alone rather than widened into this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rerun-rmack
force-pushed
the
rmack/fri-runs-map
branch
from
August 18, 2026 13:26
0390d2c to
7e28895
Compare
rerun-rmack
marked this pull request as draft
August 18, 2026 13:27
rerun-rmack
marked this pull request as ready for review
August 18, 2026 13:34
amunra
pushed a commit
that referenced
this pull request
Aug 19, 2026
`RowAddrRemap` is now on this branch (upstream lance-format#7237, backported in #43), but `open_frag_reuse_index` was never converted to it -- not here, and not on upstream main or v10.0 either. It built a `HashMap<u64, Option<u64>>` per reuse version with one entry per remapped row, on every index open, and cached the result, so readers pay for it. On a large production payload that is hundreds of millions of entries and tens of gigabytes resident, taking minutes to build -- enough to OOM the pod that opens the index. Against the same payload, `RowAddrRemap::compact` builds in a fraction of a second and well under a gigabyte, and agreed with the map on every address probed. The only differences are offsets past a fragment's `physical_rows`, where the map reports absent and compact reports deleted; that is compact's documented behaviour, and those addresses are not rows. This supersedes the hand-shaped variant in #38. With lance-format#7237 backported, this branch takes the same patch release-9 does instead of a release-8-specific one. Three additions to the remap module: `Debug` and `DeepSizeOf` for `RowAddrRemap`, plus the `num_groups`/`num_fragments` accessors those use. `FragReuseIndex` derives `Debug` and implements `DeepSizeOf`, and now stores these, so both are required of them. `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 production caller does. The follow-up commit removes that rebuild entirely. Reworded when porting: branch references updated for release-8, specific payload figures generalised, and phrasing that described this tree relative to another copy of it removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 3c9220e)
amunra
added a commit
that referenced
this pull request
Aug 20, 2026
…f a per-row HashMap (#44) * Build the fragment-reuse index open path on RowAddrRemap `RowAddrRemap` is now on this branch (upstream lance-format#7237, backported in #43), but `open_frag_reuse_index` was never converted to it -- not here, and not on upstream main or v10.0 either. It built a `HashMap<u64, Option<u64>>` per reuse version with one entry per remapped row, on every index open, and cached the result, so readers pay for it. On a large production payload that is hundreds of millions of entries and tens of gigabytes resident, taking minutes to build -- enough to OOM the pod that opens the index. Against the same payload, `RowAddrRemap::compact` builds in a fraction of a second and well under a gigabyte, and agreed with the map on every address probed. The only differences are offsets past a fragment's `physical_rows`, where the map reports absent and compact reports deleted; that is compact's documented behaviour, and those addresses are not rows. This supersedes the hand-shaped variant in #38. With lance-format#7237 backported, this branch takes the same patch release-9 does instead of a release-8-specific one. Three additions to the remap module: `Debug` and `DeepSizeOf` for `RowAddrRemap`, plus the `num_groups`/`num_fragments` accessors those use. `FragReuseIndex` derives `Debug` and implements `DeepSizeOf`, and now stores these, so both are required of them. `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 production caller does. The follow-up commit removes that rebuild entirely. Reworded when porting: branch references updated for release-8, specific payload figures generalised, and phrasing that described this tree relative to another copy of it removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 3c9220e) * Write the fragment-reuse remap comments for a general audience The comments added with the compact index-open path described this codebase relative to another copy of it, and cited one deployment's numbers. Neither survives being read by someone who does not share that context, and the figures date the comment as soon as the payload changes. Rewritten to state the properties instead: * `RowAddrRemap`'s hand-written `Debug` now says why it is not derived -- the payload is unbounded bitmaps and maps, so a derived impl would print the whole remap -- rather than describing which copy of the tree needs it. * `FragReuseIndex::row_addr_maps` and `open_frag_reuse_index` now contrast the two memory profiles directly: a materialized map holds one entry per rewritten or deleted row and so grows with the rows compaction has touched, while the compact form grows with fragment count. That is the reason the compact form is built on a cached read path, and it stays true independent of any one dataset. * `FragReuseIndex::new`'s note on `Direct` drops the `O(#rows)` shorthand for the same phrasing. * `remap_index`'s comment no longer claims no caller reaches it, which was both scoped to one deployment and misleading: the function's purpose is to consume the reuse index, so its input is by construction the largest one available. It now records that this is the last place whose memory grows per row, and that composing the per-version remaps would remove it. `CompactRowAddrRemap`'s `num_groups`/`num_fragments` also move into the existing `impl` block. They sat in a second one purely to keep them below the marker; with the marker gone the split had no reason to exist. They now sit next to `is_empty`, the other query about the structure's shape. No behaviour change: comments, and moving two methods between `impl` blocks on the same type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * Implement DeepSizeOf for RoaringBitmap and use it Two places approximated a roaring bitmap's memory by its serialized size, each with its own copy of the reasoning: `RowAddrRemap` in lance-core and `RowAddrSelection` in lance-select. Roaring does not expose its resident allocation and keeps its containers private, so an approximation is unavoidable -- but it belongs in one place, and `DeepSizeOf` is defined in this workspace, so implementing it for a foreign type is allowed. `deepsize.rs` already does that for `str`, `String`, the atomics, `dyn Array` and `RecordBatch`. With the impl in place, `RowAddrRemap`'s hand-rolled estimate is replaced by deriving `DeepSizeOf` on `CompactRowAddrRemap` and `GroupRemap`. The derive walks `Vec` -> `HashMap` -> tuple -> `RoaringBitmap`, all of which already had impls, so the arithmetic disappears and the result improves: the previous version charged fixed constants per entry and ignored the hash maps' and vectors' spare capacity, which the container impls account for. The new impl documents how close the approximation is, checked against roaring's internals rather than assumed. Each container type is laid out the same way in memory as on disk: an array container is a `Vec<u16>` at 2 bytes per value either way, a bitmap container is a `Box<[u64; 1024]>` against a fixed 8 KiB, and a run container is a `Vec<Interval>` at 4 bytes per run against `RUN_ELEMENT_BYTES` of 4. So the serialized size covers the payload closely. What it misses is the `Vec<Container>` those payloads hang off, which costs tens of bytes per container against a 4-byte serialized descriptor, so an allowance for it is added back. That sum errs high -- it double-counts the descriptor, and most bitmaps also carry a 4-byte offset per container in the header -- which is the safer direction when the figure is what a byte-bounded cache charges an entry at admission. The remaining shortfall is the array and run vectors' spare capacity, which is not observable through roaring's public API. `RoaringBitmap::statistics()` would expose the capacities, but it reports array bytes as `capacity * size_of::<u32>()` for a `Vec<u16>` and bitmap bytes as a bit count, so deriving the payload from it would mean depending on those quirks. Only its container count is used, which is sound. Tested from both directions: the same cardinality spread across four containers must cost more than packed into one, and more values in a single container must cost more than fewer. Test asserts the derive reaches the bitmaps rather than stopping at the enum: two remaps differing only in how many rows their bitmaps hold must report different sizes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * Test the fragment-reuse index open path The open path had no direct coverage: the change from a per-row map to the compact form was verified only by the module's own unit tests one layer down, and by whatever exercised it incidentally. `open_frag_reuse_index` turns out to be testable without a dataset -- it takes `FragReuseIndexDetails` directly -- so these build details in memory, serialize the row-address treemaps the way the real payload does, and open the result. Scenarios chosen for where the positional pairing is most likely to go wrong: * moved, deleted and untouched addresses each resolving distinctly, including the difference between a covered-but-emptied fragment (deleted) and a fragment no group mentions (unchanged) * agreement with `transpose_row_ids_from_digest` across every real address, for ascending old fragments -- the case where the positional and address-ordered pairings must agree * non-ascending `old_frags`, where the two pairings disagree. This input is not reachable from any current writer: `build_manifest` sorts the manifest's fragment list by id after every operation, documented as an invariant of `Manifest::fragments` and relied on elsewhere, so a rewrite group's fragments always arrive ascending and read order equals address order. Pinned anyway, because it records which pairing is correct -- for this input the map form overwrites two live rows' mappings with `None` and reports them deleted, where the positional form is right. A latent bug in the code being replaced, not a live one * chains of 2, 8 and 32 versions, entering at the head and midway * deletion mid-chain being terminal, with a later version left unconsulted * zero-row new fragments not consuming a position * several rewrite groups sharing one version, with independent positions * empty details, and a version carrying no groups * four inconsistent payloads that the map form accepted silently -- row counts disagreeing either way, new fragments out of write order, and a rewritten address outside the group's old fragments -- now rejected at open * a corrupt treemap surfacing as an error rather than a panic On the `FragReuseIndex` side: composition over compact rounds, `remap_row_id`, `remap_row_ids_record_batch` keeping values paired with their remapped addresses, `remap_row_ids_roaring_tree_map`, and a 32-round chain. Also pins that a chain tolerates a `Direct` link. Nothing in-tree builds a mixed chain -- `new` produces all `Direct`, the open path all `Compact` -- but `new_from_remaps` and the public field permit one. Asserted against absolute expectations rather than against an all-compact chain, since a differential check would also pass against a stubbed `remap_row_id`, and it records that the two forms are chainable rather than interchangeable: a `Direct` link knows only the addresses it lists, so an unlisted offset in a covered fragment reads as untouched where the compact form reports it deleted. Four scenarios came out of review, covering branches the first pass missed: * that the open path yields `RowAddrRemap::Compact` at all. Every lookup below answers identically for a materialized map, so without this a revert to `transpose_row_ids_from_digest` would restore the memory blowup and stay green * a 200k-row fragment whose addresses are run-optimized before serializing, which is the shape `rewrite_files` writes. Every other payload here is a small array container, so `RoaringBitmap::rank` was never exercised on run or bitmap containers or on offsets past 65535 * five new fragments in one group, so the binary search in `compute_new_addr` lands strictly inside the range list. With one or two ranges a mid-list off-by-one passes * an emptied old fragment in the middle of `old_frag_ids` rather than last, where mis-charging its rows to the running position would shift the fragments after it `remap_row_ids_record_batch` is now parameterized over both column layouts in use: `row_id_idx` is 1 for scalar indices and 0 for vector storage, and only the former was covered. `remap_row_addrs_tree_map`, the wrapper with the most callers in-tree, was untested and now is. Dropped from the first pass: chain depths of 2 and 8, which pin nothing 32 does not; a second 32-round chain test one layer down; and two rejection cases that duplicate `row_addr_remap.rs`. The row-count cases are kept because that validation has no coverage there, and they now assert the error is the one they name rather than any error at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * Keep opening fragment-reuse payloads that record no rewritten rows Positional remapping requires a group's rewritten-row count to equal the row count of its new fragments, and rejects the group otherwise. The per-row map it replaces had no such check -- its `zip` silently truncated -- so a payload that violates the rule used to load and now does not. That is reachable, and not only in theory. Lance 0.30.0 through 4.0.0-beta.6 wrote exactly this shape for a stable-row-id dataset whose index remap was deferred: an empty set of rewritten addresses, alongside new fragments carrying real row counts. The writer stopped doing it in 4.0.0-beta.6's successor, which made row addresses mandatory on that path, but the payloads it wrote are on disk. The consequence would not have been confined to a remap. `open_frag_reuse_index` runs inside `load_indices`, so the error surfaces in scan planning, `validate()`, and every commit -- a dataset that opens today would stop opening. Exactly the memory-problem-into-availability-problem trade this change set exists to avoid. So a group recording no rewritten rows but non-empty new fragments now drops those new fragments, leaving every fragment it covers resolving to deleted. That is what the per-row map produced for the same input: with no addresses to pair, it emitted every covered address as missing. The rows are unreachable through the index either way until it is rebuilt; only the failure mode would have been new. Logged at warn so the payload is visible rather than silently reinterpreted. A group that genuinely deleted everything carries no new fragments, so it is untouched by this: the shapes are distinguished by the new fragments being present. Both cases are tested, as is the checked-in v6.0.0-beta.3 dataset under `test_data/fri_straddle_pre_6610`, which continues to open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * Correct the ordering docs on both fragment lists `MissingAddrs::new` has said "fragments is not guaranteed to be sorted by id" since 2023-11-03 (lance-format#1511). That was true when written, and stopped being true 132 days later: lance-format#2075, "force fragments to be stored in the manifest in id-order", made `build_manifest` sort the list after every operation, which `Manifest::fragments` documents as an invariant and which `Dataset::validate` and `Manifest::fragments_by_offset_range` both rely on. The comment is still correct about this type -- it does tolerate an unsorted list, deliberately -- but read as evidence that unsorted input arises in practice it is badly misleading, and it cost real time. The note now says what the tolerance is and is not, and spells out what unsorted input actually produces here: with `row_addrs` in ascending address order, which is how a serialized `RoaringTreemap` iterates, the rows of an out-of-order fragment are reported missing and their real mappings overwritten. `GroupRemap::new` gains the matching note on the other side. Its two fragment lists are treated differently and nothing said why: `old_frag_ids` carries no ordering requirement, because positions are assigned by walking it, so every order is handled and none rejected; `new_frags` must be ascending, because `compute_new_addr` accumulates row counts in that order, and a violation is rejected. Worth stating together, since the natural question on reading the one check is why there isn't another. No validation added for `old_frag_ids`. It would reject payloads this code already handles correctly, which is the same availability trade the previous commit removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * Test that old fragment order is followed whatever it is The previous commit documented that `old_frag_ids` carries no ordering requirement: positions are assigned by walking the list, so any order is handled and none is rejected. Relaxing a constraint in a comment is worth little without tests over the space it opens up, and the existing coverage was thinner than it looked. What was there: one two-element descending case with every row rewritten, and a three-element non-monotonic list whose middle fragment was the emptied one. Every expectation was hand-written. Added an `rstest` over four permutations -- ascending, descending, rotated, and a scrambled sparse set -- with each fragment keeping two of three rows so deletions interact with the ordering, and the output split across two fragments so the range search does too. Expectations are derived from the contract rather than written out: the k-th kept row in list order occupies the k-th slot across the new fragments in their order. Checked the tests actually bite, by sorting `old_frag_ids` inside `GroupRemap::new` and confirming what fails: the descending, rotated and scrambled cases, the existing read-order test, and `test_compact_lookup`. The ascending case correctly survives, since sorting a sorted list changes nothing. That exercise also showed the pre-existing three-element case was never really ordering coverage: `[0, 7, 1]` sorts to `[0, 1, 7]`, and because fragment 7 is the emptied one the two live fragments keep their relative order, so it passes either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * Run the btree frag-reuse reconstruct test against both remap forms Upstream lance-format#7237 parameterized four index types over `Direct` and `Compact` and left the rest on `Direct` only. One of those left behind matters here: `test_btree_index_state_reconstruct_applies_frag_reuse_index` is the only test that drives a `FragReuseIndex` through a real index reconstruct, and it built one from a materialized map -- the form the open path no longer produces. Now an `rstest` over both. The two cases move a row differently, because `Compact` cannot name an arbitrary destination, only a position in a new fragment: the direct map sends row 0 to 5000, while the compact form rewrites all 1000 rows of fragment 0 into fragment 1 in order, sending row 0 to fragment 1 offset 0. Both land outside the original `[0, 1000)` range so neither collides, and rewriting every row keeps the compact case a faithful analogue rather than deleting the other 999. Checked both cases depend on the remap rather than passing incidentally, by not handing the index to `reconstruct`: both fail. Six files still carry `Direct`-only remap tests -- `lance/src/index.rs`, `index/vector/ivf.rs`, `scalar/lance_format.rs`, `scalar/inverted/index.rs`, `scalar/inverted/builder.rs` and `vector/bq/storage.rs`. Extending the parameterization to those is worth doing and is not specific to the fragment-reuse index, so it belongs in its own change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * Run the all-rows-deleted remap test against both remap forms Upstream lance-format#7237 parameterized four index types over `Direct` and `Compact` and left seven test sites on `Direct` only. I parameterized all of them to see whether the compact form broke any, and it broke none -- so all but one are reverted here rather than carried. The one kept is `index.rs::test_remap_empty`. It is the only test where the two forms run genuinely different logic in a decision that changes the outcome: `remap_index` returns `Keep` instead of rewriting when `fully_deleted_fragments` matches the index's fragment bitmap, and the two arms compute that set differently -- `Direct` infers it from whichever keys the map happens to hold, `Compact` reads it off the fragments its groups cover. `Direct`'s inference is unsound for a partial map, over-claiming a fragment as fully deleted on the evidence of one row; the compact arm cannot make that mistake. Nothing else exercised it through a real caller. The rest were dropped because `fully_deleted_fragments` and `affected_fragments` are the only methods any consumer calls beyond `get`, and the sole non-test caller of either is the branch above. Every other remap consumer -- bitmap, bq storage, inverted, btree, pq -- only calls `get`, so the difference between the forms at those sites lives entirely inside `RowAddrRemap::get`, which is unit-tested directly one layer down. Both `bq/storage.rs` cases were the clearest example: each leaves exactly 49 surviving rows, so the repack path under test sees an identical input shape and only the row-id values differ. Also not parameterized: `index/vector/ivf.rs`. Its `build_mapping` leaves a third of the rows absent so they pass through unchanged, and `check_index` asserts that third stayed put. `Compact` cannot express that, since an unlisted offset in a covered fragment reads as deleted, so covering it would mean restructuring what the test asserts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * Let a reader choose how the fragment reuse index is expanded Compaction picks between the materialized and compact remaps with `IndexRemapMode`; the reader had no equivalent, so `open_frag_reuse_index` built one form unconditionally. That leaves the other arm of the enum dead on the read path, and leaves a deployment no way to change form short of a new build. `ReadParams::frag_reuse_remap_mode` now selects it, with `DatasetBuilder::with_frag_reuse_remap_mode` to set it and the `LANCE_FRAG_REUSE_REMAP_MODE` environment variable supplying the default when present. This follows `DecoderConfig::cache_repetition_index`, which is how this crate already exposes a reader-side switch: a public field whose `Default` consults an environment variable through a `OnceLock`, so the variable is read once per process and the API remains the way callers select behaviour. **The default is `Direct`, which preserves the behaviour a reader had before the compact form existed.** Picking up a Lance carrying this change is therefore not itself a behaviour change; a caller that wants the compact form asks for it. That matters because the compact form is not a strict improvement -- it trades memory that grows with rows for memory that grows with fragments, at the cost of more work per lookup -- so which one suits depends on the dataset, and the choice belongs to the caller rather than to a release note. The mode is part of the fragment reuse index's cache key. Without that, two datasets sharing a `Session` and choosing differently would serve each other an entry built the other way, and the forms are not interchangeable: they resolve some addresses differently. Threaded the way `file_reader_options` already is -- `ReadParams` to `Dataset` to the open site -- so the plumbing follows an existing path rather than inventing one. Two things that path does not make obvious, both regression-tested: * the builder keeps its own copy of the field, so its default has to consult the same environment-aware function rather than `IndexRemapMode`'s derived default -- with the derive it would ignore the variable, and would drift if `#[default]` ever moved * `with_read_params` copies fields one at a time, so a new one is silently dropped unless added there too Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * Test the reader-side remap switch end to end, including what it costs The switch had no test through the public API: the earlier ones passed a mode to an internal function, and nothing exercised `DatasetBuilder::with_frag_reuse_remap_mode` or the plumbing behind it. Two tests, both building a multi-fragment dataset with a scalar index and then compacting several times with the remap deferred, so the index is never rewritten and its stored addresses have to be resolved through an accumulated reuse index at query time -- which is what makes these exercise the reader rather than compaction. The first reopens through the builder in each form and checks they answer identically: the same counts before compaction and after, and the scalar index still serving the scan. It also asserts the reopened dataset carries the mode that was asked for, because without that a no-op setter would pass -- the two forms agree on every count, which is the property being checked, not evidence either was selected. The second measures the difference rather than arguing it. Opening the same dataset each way and reading `Dataset::cache_size_bytes` around a query, the materialized form grows the cache by about 4.4 MB and the compact form by about 140 kB -- roughly thirty times, on eight fragments of five thousand rows with three compaction rounds. That is a public API over the same estimate the byte-bounded cache charges at admission, so it measures what the cache believes rather than resident set size; the gap is wide enough that the distinction does not change the conclusion. The assertion is a tenfold margin against a measured thirtyfold, and it also requires the materialized form to have grown at all, so a test that cached nothing cannot pass. Both were checked by mutation: neutering the builder's setter fails the first, and forcing the open path back to the materialized form fails the second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * Keep the two remap forms apart in a shared session's index cache What an index caches is not the index as stored. With the remap deferred its row addresses are stale, and the load path translates them through the fragment reuse index as it reads. So the cached state is the index *as translated*, and the two forms do not translate identically -- an offset past a fragment's `physical_rows` is deleted under one and untouched under the other. The cache prefix identified which reuse index did the translating, `{index}-{fri}`, but not which form. A `Session` is shared state by design, so two datasets pooling one could disagree about the form, and the second would be served state the first had already translated: the mode became advisory, decided by whichever opened first. Measured on a shared session, opening the second form added one cache entry rather than a set -- it built its own reuse index and then read the other's work. `GlobalIndexCache::for_dataset_with_remap_mode` puts the form in the per-dataset prefix, so everything beneath it -- the reuse index and every index's own state -- is partitioned without touching `for_index` or any of its callers. The same open now adds a full set of entries instead of one, so each form does its own translating. Partitioning here rather than per index also removes the need for the mode in `FragReuseIndexKey`, which is reverted: one partition point instead of two. Costs nothing in the ordinary case. One form in use means one prefix and the cache behaves exactly as before; two forms cost a second copy of the state for the datasets that differ, which is memory rather than correctness. `load_manifest` deliberately keeps the unpartitioned prefix: the index metadata it caches is the manifest's own list and does not depend on the form. If `Direct` is eventually removed, one form remains and this collapses back into `for_dataset`. The accompanying test opens the same dataset version both ways in one session, in both orders, and checks the answers agree. It passes with or without this change, because the forms agree on every address a writer can produce -- so it is regression cover for the sharing working at all, not a guard on the partitioning. The partitioning was verified by cache entry counts instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * test: turn the deadlock test's prints into assertions The fragment-reuse deadlock test printed its compaction metrics, the index names it found and a success line, none of which a test run checks. Two of them were guarding something worth asserting, so assert it: compaction must have rewritten fragments, or no reuse index exists and the path under test is never entered; and the index names belong in the failure message of the assertion that looks for the reuse index, not on stdout ahead of it. The remaining two were progress narration. The timeout is what decides the test, so the elapsed time it reported was never read. `println!` is denied workspace-wide (`clippy::print_stdout`), so this also takes the file off the list of reasons `cargo clippy --all --tests --benches -- -D warnings` fails, and sorts its imports so `cargo fmt --all --check` passes. Both were missed because `.github/workflows/rust.yml` triggers on `release/**`, which does not match this branch's base. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * fix: keep the manifest's index list out of the remap-partitioned cache Partitioning the dataset's index cache by remap form moved every key under a `{uri}/{form}` prefix, including the manifest's own index list. That list is read straight from the manifest and is not translated through the fragment reuse index, so it is identical under either form -- and `load_manifest` prefetches it while decoding the manifest, before any form is known, at the unpartitioned prefix. Producer and consumer therefore stopped agreeing. `load_manifest` wrote the entry under `{uri}/` and `load_indices` looked for it under `{uri}/Direct/`, so the prefetch became a write nobody read: every `load_indices` missed and re-read the index section from the manifest object. That is scan planning, `validate()` and every commit, and the orphaned entries stayed admitted against the byte-bounded cache while being unreachable. `test_load_indices` covers exactly this -- "we should have opportunistically cached the indices in memory already" -- and had started failing. Give the dataset a second handle for entries that are manifest-derived rather than translated, and put the index list there on both sides. The partitioned handle keeps everything whose contents the form can change, so it stays safe by construction: anything reached through it is partitioned without the caller having to remember. The shared-session test also gains the assertion it was missing. It compared row counts, which agree between the forms for any payload a real writer produces, so it passed with the partitioning deleted outright. It now checks the form of the reuse index each reader is served, and fails in both directions under that mutation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * fix: correct comments that state the opposite of the code, and two vacuous tests Four public methods had lost their documentation. Both new remap-mode setters were inserted between an existing doc comment and the method it documented, so `ReadParams::file_reader_options` and `DatasetBuilder::with_read_params` ended up undocumented while the new methods rendered under someone else's summary line -- which is the line rustdoc shows in the method list. Three comments said something untrue: - "It cannot fail" sat six lines above a `?` on a treemap deserialize. The narrower claim is what was meant: the materialized form does not reject a payload whose row counts disagree, where the compact form does. - `row_addr_maps` was documented as built compact, but the form became a reader choice defaulting to direct, so the field doc stated the opposite of what ships. - A test comment said `old_frag_ids` imposes no ordering requirement, against a module header stating the order is load-bearing. What is absent is validation, not the requirement; a reader who believed the comment would pass an arbitrary order and get silently wrong addresses. Two tests asserted things that could not fail. The roaring size test compared totals across bitmaps with different container counts, but a serialized bitmap already carries a per-container descriptor, so the assertion held with the in-memory container allowance removed entirely. It now compares the allowance itself. The builder default test claimed to catch the builder taking the derived default rather than the environment-aware one, but with the variable unset the two are the same value, so it could only ever catch `#[default]` moving. The parsing is now split out of the `OnceLock` -- which reads the environment once per process, so no test could vary it -- and covered directly, including the unparseable value that warns and falls back. The builder test keeps its wiring check and drops the claim it could not support. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK * test: run the deferred-remap catch-up against both remap forms Every test that reached `remap_column_index` ran at the default form, and the three tests that select a form are read-only after opening. So the branch that exists because the compact form cannot enumerate its keys was never executed -- the half of the change written for `Compact` had no coverage at all. This is the sequence the compact form exists for: open `Compact` so the reuse index does not cost one entry per rewritten row, let maintenance rewrite the index, then trim the reuse index away. The load-bearing assertion is the last one. Trimming the reuse index to zero versions only shows maintenance ran, and stays true even when the remap wrote a wrong index, because trimming is driven by index metadata rather than index contents. A reader opened after the trim has nothing left to translate through, so it can only answer correctly if the rewritten index really holds current addresses. Emptying the composed remap fails exactly that assertion, in both forms, with the trimming assertion still green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXueHshgpL7dw1adLsfTUK --------- Co-authored-by: Ryan Mack <rmack@rerun.io> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Problem
open_frag_reuse_indexbuilds aHashMap<u64, Option<u64>>per reuse version with one entry per remapped row. It runs on every index open and the result is cached, so readers pay it.Measured on a production payload: 676,592,102 entries. 88 MB on disk becomes 26.8 GB resident, 40.5 GB peak during construction, and takes 144 s to build. That OOMs a 60 GiB pod, which is how we found it.
Fix
Upstream already built the right structure. lance-format#7237 ("introduce RowAddrRemap structure to avoid remap OOM caused by HashMap", merged 2026-07-03) added
RowAddrRemapwith aCompactrank/select variant that is O(#fragments). It applied it to the scalar-index remap consumers and left the fragment-reuse index-open path on the hashmap, which is the path that OOMs us. That is still true onmainand every release branch through v10.0.So this uses their structure rather than inventing one.
Two commits:
RowAddrRemapfrom release-9.0.0, byte-identical toorigin/release-9.0.0, verified by diff. Reviews as a pure import; diffs clean when we move to 0.16.RowAddrRemap::compacttakes exactly what aFragReuseGroupalready holds, so the conversion is mechanical.FragReuseIndex::newstill accepts maps and stores them asRowAddrRemap::Direct, so existing callers are unaffected.What we had to add on top of the backport
Marked as fork-local at the bottom of the backported file so the delta against release-9 stays visible:
DebugandDeepSizeOfforRowAddrRemap, plus thenum_groups/num_fragmentsaccessors they use.FragReuseIndexneeds both; upstream does not yet, precisely because it still holds hashmaps there. They will hit this when they convert the open path.remap_column_indexcomposed a map by enumerating keys, andCompactdeliberately 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 per-row maps from the details. Still O(rows), but only tests reach it in-tree and nothing in redap does.Testing
Upstream's 5 module tests pass unmodified. lance-table 107, lance
frag_reuse3,optimize::95,remap30. Clippy and fmt clean.Open, and why this is still draft
Not yet run against the real 88 MB payload. Two things need measuring there before merge:
CompactRowAddrRemap::getreports any offset in a rewritten fragment that is not in the bitmap as deleted, including offsets pastphysical_rows, where the hashmap reports absent. Semantics differ from today on those edge cases.GroupRemap::newhard-errors if new fragments are not in ascending id order, or if the rewritten-row count does not match the new fragments' total. On the read path that turns a bad committed payload into a permanent query failure. This connects to the open upstream fix(compaction): preserve row order across parallel tasks lance-format/lance#8400 about parallel compaction tasks arriving out of order, and to the fact that the sortedness check atoptimize.rs:634is adebug_assert!.If a payload we have already committed trips either condition,
open_frag_reuse_indexfails every query on that dataset, which would be worse than the OOM. Measuring that is the remaining gate.Superseded
An earlier revision of this PR implemented a run-length structure of our own (
RowAddrRunMap). Dropped in favour of upstream's. Thanks to the review that surfaced lance-format#7237.