Conversation
…sher `FragReuseIndex::remap_row_id` probes `CompactRemapStep::frags` once per remap step per row address. The map is keyed by a u32 fragment id and was on SipHash, so on a synthetic 500M-row chain across 26 reuse versions that is billions of cryptographic hashes of a 4-byte internally generated key on the critical path to the bucket load. Profiling put ~35% of consolidation CPU in `hash_one` plus `sip::Hasher::write`; that was measured on a two-probe variant of this module where every lookup hashed twice, so the saving on the single-probe layout here is expected to be smaller. Replace it with a module-private multiply-based hasher over the golden-ratio constant. `finish` folds the high half down because multiplication only carries upward while hashbrown picks the bucket from the low bits. The build-time `per_frag` map takes the same hasher; the duplicate-fragment `HashSet` and the `Direct` step's u64-keyed map stay on the default hasher. `DeepSizeOf for HashMap<K, V>` is widened to `HashMap<K, V, S>` so `CompactRemapStep::deep_size_of_children` keeps compiling.
A wrong row-address translation does not crash, it silently points an index at the wrong physical row, so the compact remap is compared address by address against a materialized old-to-new map over randomized rewrites. A mutation to the compact step's rank arithmetic fails it. Also pins the parts of the integer hasher that are easy to regress: the fold in `finish` that keeps power-of-two-strided fragment ids out of one bucket, the seedless per-key determinism that keeps a key reachable across a resize, and the documented composite-key collision. `proptest` is added back to `lance-core`'s dev-dependencies for the randomized check.
amunra
marked this pull request as ready for review
September 17, 2026 13:19
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The change removes a measured hashing bottleneck from the per-row/per-version remap path without changing persisted data or row-address semantics. Keeping the hasher module-private and restricted to single-u32 keys is a proportionate solution: a dense table would waste memory for sparse fragment IDs, while a general-purpose dependency would add cost without a stronger contract. The randomized equivalence, distribution, ordering, deep-size, and fragment-reuse integration coverage support the change.
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
FragReuseIndex::remap_row_idtranslates a stored row address through every fragment-reuse version in the chain. EachCompactRemapStepprobesfrags: HashMap<u32, OldFragmentRemap>once per address. The map is keyed by au32fragment id and was on Rust's default SipHash, so consolidating indexes over hundreds of millions of rows and tens of reuse versions performs billions of cryptographic hashes of a 4-byte, internally generated key, and the hash sits directly on the critical path to the bucket load.Profiling on a two-probe variant of this module, where every lookup hashed twice, put ~35% of btree consolidation CPU in
hash_oneplussip::Hasher::write. The module here makes one probe per step, so the saving is expected to be smaller, but the hash is still the first thing every lookup does.Change
A module-private
IntHasher: multiply by the golden-ratio constant, then fold the high half into the low half infinish. It is exposed astype IntMap<K, V> = HashMap<K, V, BuildHasherDefault<IntHasher>>and used forCompactRemapStep::fragsand the build-timeper_fragmap inGroupRemap::new_with_old_frags. The duplicate-fragmentHashSetand theRemapStep::Direct(HashMap<u64, Option<u64>>)map stay on the default hasher. No new runtime dependency.The fold matters. Multiplication only carries upward, so without it the low bits of the hash, which hashbrown uses to pick the bucket, would ignore the high bits of the key, and fragment ids strided by a power of two would all land in one bucket. On a 2048-bucket table with 1024 keys strided by 2^20 that is 1 distinct bucket without the fold and about 800 with it.
Dropping SipHash is safe here because the keys are fragment ids the system generates itself. They are never attacker-supplied, so the hash-flooding resistance SipHash buys is worth nothing on this path.
Known limitation:
write_u32/write_u64/write_usizereplace the hasher state rather than mixing into it, so a composite integer key would collide on its last field alone. That is correct but slow (Eqstill decides), so the type is documented as single-integer-key only, kept module-private, and the behaviour is pinned by a test so a future key-type change shows up as a deliberate decision.DeepSizeOfchangeimpl<K, V> DeepSizeOf for HashMap<K, V>is widened toimpl<K, V, S>, becauseCompactRemapStep::deep_size_of_childrencallsself.frags.deep_size_of_childrenand would otherwise stop compiling. The impl body never touchedS, so the widened form is the more accurate one.DeepSizeOfislance-core's own trait anddeepsize.rsholds the onlyDeepSizeOf for HashMapimpl, so the widening is internal to the trait and has no downstream impact beyond type inference in the rare case thatSis not already fixed.One side effect:
BuildHasherDefaultis a ZST whereRandomStateis 16 bytes, sosize_of::<HashMap<..>>()drops from 48 to 32 and the reported deep size of a remap shrinks by 16 bytes per map. That moves cache accounting toward accuracy, not away from it.Public API
One existing impl is widened,
rust/lance-core/src/deepsize.rs:impl<K: DeepSizeOf, V: DeepSizeOf> DeepSizeOf for HashMap<K, V>impl<K: DeepSizeOf, V: DeepSizeOf, S> DeepSizeOf for HashMap<K, V, S>Needed because the map this PR re-hashes no longer uses the default hasher.
DeepSizeOfis this crate's own trait and this is the only impl of it forHashMap, so the orphan rule leaves no downstream impl that could conflict; see the section below. The newIntHasherandIntMapare module-private.Numbers
Synthetic dataset, 500M rows, 26 index segments, a fragment-reuse chain of 26 versions covering all rows, cold cache, measured on the two-probe variant of this module with both sides otherwise identical:
Identical end state in both runs (26 segments -> 1, reuse chain drained), RSS unchanged. A control run with no reuse chain puts the floor at 32.6s, so this recovered roughly 80% of the remap overhead on that variant.
Tests
A wrong row-address translation does not crash; it silently points an index at the wrong physical row. So the second commit adds a proptest that compares the compact remap against a materialized old-to-new map address by address over randomized rewrites, with fragment ids drawn from dense, power-of-two-strided and sparse shapes. It is mutation-checked: perturbing the rank arithmetic in
CompactRemapStep::getfails it.Alongside it: the fold in
finish(power-of-two-strided fragment ids must spread over buckets), seedless per-key determinism (a key must stay reachable across a resize), the composite-key collision above, and insertion-order independence of every set-shaped output.proptestis added back tolance-core's dev-dependencies for the randomized check; it was there at v10.0.0.Validation:
cargo fmt --all -- --check,cargo clippy --all --tests --benches -- -D warnings,cargo test -p lance-core,cargo test -p lance-table, all clean.Compatibility
No file-format or on-disk change. The hasher only affects in-memory lookup. Iteration order of the affected maps changes; the set-shaped outputs are order-independent (covered by the insertion-order test), and the only place the order shows is the fragment-id list inside one
invalid_inputerror message.Tracking: Ported from rerun-io#60.