perf(cache)!: use fixed-size cache keys#7878
Conversation
Replace logical string keys with domain-separated BLAKE3-128 identities to remove long-prefix hashing and simplify persistent backends. Co-authored-by: Cursor <cursoragent@cursor.com>
Preserve the existing examples and section markers while adapting the cache implementation. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
github-actions
Bot
added
A-python
📝 WalkthroughWalkthroughThis change replaces string-based cache keys with versioned typed BLAKE3-derived keys, updates cache backend and lifecycle APIs, migrates cache-key implementations across Lance components, removes IVF prefix persistence, and adds serializing-backend, allocation, and benchmark coverage. ChangesTyped cache key migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| .unwrap_or(u32::MAX) | ||
| entry_weight(key, entry.size_bytes) | ||
| }) | ||
| .support_invalidation_closures() |
There was a problem hiding this comment.
no longer needed, and saves us some (accounting) space inside moka, but I can add it back if we need it for some reason
| /// - **type_name**: distinguishes different value types stored under the same | ||
| /// user key (e.g. `"Vec<IndexMetadata>"`) | ||
| #[derive(Clone, Debug, Hash, PartialEq, Eq)] | ||
| pub struct InternalCacheKey { |
There was a problem hiding this comment.
I opted for reworking InternalCacheKey directly instead of having 2 different formats due to the the amount of breaking API changes this PR was already introducing, and the tests assert the stability.
This does mean that all existing persistent backend will see new keys on deployment, but the contract is explicit that going forward format changes are expected and should lead to cache misses (with persistent backends responsible for cleaning up the stale keys - likely through TTLs).
If we really want to support both layouts and have the caller pick, I can add that back in (still requires breaking API changes) - at the cost of some wrapper structs that we'll need to maintain. IMHO only really worth it if we envision a future where we'll want to allow different cache key layouts.
Account for large entries without u32 truncation and include storage base in deletion keys. Strengthen migration, benchmark, and persistence proofs discovered during review. Co-authored-by: Cursor <cursoragent@cursor.com>
| //! protocol produces cold misses instead of aliases. Persistent or tiered | ||
| //! backends can route serializable values with [`CacheCodec::type_id`]. | ||
| //! | ||
| //! Prefix invalidation and key inventory are intentionally not part of this |
There was a problem hiding this comment.
can remove this paragraph, if we don't need to carry historical context
| .try_into() | ||
| .unwrap_or(u32::MAX) | ||
| .max_capacity(capacity_weight) | ||
| .weigher(move |key: &InternalCacheKey, entry: &MokaCacheEntry| { |
There was a problem hiding this comment.
bit of a drive-by here to not cap an entry weight to u32::MAX (which I know we've said before that entries should not get this large) but since we've seen this in the past with our indexes lookup files, I think this is probably worth it, just as defensive posturing for the future?
Represent stable field discriminants as a repr(u8) enum so their relationship and one-byte encoding are explicit without changing key bytes. Co-authored-by: Cursor <cursoragent@cursor.com>
| @@ -0,0 +1,365 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
This file can be removed before merging, if we want.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance-index/src/scalar/bitmap.rs (1)
150-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why
write_keyusesrow_offsetinstead ofvalue.
write_keyintentionally diverges fromkey()here:key()hashesvalue, butwrite_key()only encodesrow_offset. This is correct —row_offsetis a bijective stand-in forvaluewithin a single loaded index (index_map: BTreeMap<Value, usize>) — and it's covered bytest_bitmap_cache_key_uses_row_offset_identity. However, every otherCacheKey/UnsizedCacheKeyimpl touched in this PR mirrors the exact fields used inkey(), so this is the one outlier. A short comment would prevent a future refactor from "fixing" this back to hashingvalue(which would be more expensive and unnecessary) or breaking the row_offset-uniqueness invariant it relies on.📝 Suggested comment
fn schema() -> CacheKeySchema { CacheKeySchema::new("lance.scalar.bitmap-row-offset-key", 1) } + // `row_offset` is a bijective stand-in for `value` within a loaded index + // (each entry in `index_map` has a unique row_offset), so it alone is + // sufficient physical identity — cheaper to encode than the arbitrary + // `ScalarValue`. See `test_bitmap_cache_key_uses_row_offset_identity`. fn write_key(&self, builder: &mut KeyBuilder) { builder.write_u64(self.row_offset); }As per coding guidelines, "Add doc comments to magic constants, thresholds, and non-obvious transformation functions, explaining what the value represents and why it was chosen."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-index/src/scalar/bitmap.rs` around lines 150 - 168, Add a concise comment to BitmapKey::write_key explaining that row_offset is a bijective, cheaper identity for value within a loaded index, so it preserves uniqueness without encoding value directly. Keep the existing write_u64(self.row_offset) behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rust/lance-index/src/scalar/bitmap.rs`:
- Around line 150-168: Add a concise comment to BitmapKey::write_key explaining
that row_offset is a bijective, cheaper identity for value within a loaded
index, so it preserves uniqueness without encoding value directly. Keep the
existing write_u64(self.row_offset) behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: f539033c-e139-4816-b83c-9a84c9ee9313
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
Cargo.tomlrust/lance-core/Cargo.tomlrust/lance-core/benches/cache_keys.rsrust/lance-core/src/cache/backend.rsrust/lance-core/src/cache/codec.rsrust/lance-core/src/cache/key.rsrust/lance-core/src/cache/mod.rsrust/lance-core/src/cache/moka.rsrust/lance-core/tests/cache_key_allocations.rsrust/lance-encoding/src/encodings/logical/primitive.rsrust/lance-file/src/previous/reader.rsrust/lance-file/src/reader.rsrust/lance-index/protos-cache/cache.protorust/lance-index/src/scalar/bitmap.rsrust/lance-index/src/scalar/btree.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/label_list.rsrust/lance-index/src/scalar/lance_format.rsrust/lance-index/src/scalar/ngram.rsrust/lance-index/src/scalar/registry.rsrust/lance-index/src/scalar/rtree.rsrust/lance/src/dataset/fragment.rsrust/lance/src/dataset/tests/dataset_index.rsrust/lance/src/index.rsrust/lance/src/index/vector/ivf.rsrust/lance/src/index/vector/ivf/partition_serde.rsrust/lance/src/index/vector/ivf/v2.rsrust/lance/src/session.rsrust/lance/src/session/caches.rsrust/lance/src/session/index_caches.rsrust/lance/src/utils/test.rsrust/lance/src/utils/test/serializing_cache.rs
💤 Files with no reviewable changes (1)
- rust/lance/src/index/vector/ivf/partition_serde.rs
| // type is generic over the quantizer; the proto envelope still provides | ||
| // additive evolution for the surrounding fields. | ||
| string quantizer_metadata_json = 7; | ||
| string cache_key_prefix = 8; |
There was a problem hiding this comment.
I can revert this, and just have cache_key_prefix be used, instead of removing/deprecating at the proto level.
wjones127
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/lance-encoding/src/encodings/logical/primitive.rs (2)
1744-1787: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUntrusted-metadata subtraction can underflow.
items_in_page - countedinanalyze_value_countsandflat_value_counts_iterassumes the per-chunklogvalues decoded from the on-disk metadata word buffer always sum to at mostitems_in_page. If a corrupted or mismatched file violates that (onlydebug_assert!-checked, not enforced in release), this underflows: a panic in debug builds, or a silently wrapped hugeu64in release that then feedsPrefixSums/row-mapping construction and downstream chunk lookups.Consider using
checked_suband surfacing a descriptiveError::invalid_input_source(propagated throughbuild_chunk_index's callers, which already returnResult) instead of relying on a debug-only assertion.Based on learnings, watch for corrupted/misaligned decoded metadata driving unchecked arithmetic into incorrect or huge values; prefer a fallible path that maps to a proper input-validation error instead of proceeding with unchecked arithmetic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-encoding/src/encodings/logical/primitive.rs` around lines 1744 - 1787, Make analyze_value_counts and flat_value_counts_iter fallible when subtracting counted values from items_in_page. Replace unchecked subtraction with checked subtraction and return a descriptive Error::invalid_input_source when decoded metadata exceeds the page count, rather than relying on debug_assert! or wrapping. Propagate the error through build_chunk_index and its existing Result-returning callers.Source: Learnings
1793-1855: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
assert!on file-derived data can panic on malformed input.
assert!(rep_index_data.len() % 8 == 0)at line 1815 asserts on a length read from the file's repetition-index buffer. Per coding guidelines, library code must not useassert!/panic!for fallible conditions over external/untrusted input — a malformed or truncated file would crash the reader instead of surfacing a structured error, unlike the sibling checks elsewhere in this same file (e.g. chunk-size/log-value validation) that were converted from panics toError::invalid_input_source.🛡️ Suggested direction
- assert!(rep_index_data.len() % 8 == 0); + if rep_index_data.len() % 8 != 0 { + return Err(Error::invalid_input_source( + format!( + "Repetition index buffer length {} is not a multiple of 8 bytes", + rep_index_data.len() + ) + .into(), + )); + }Note: this requires
build_chunk_index(and its callers) to returnResult, which is a larger but consistent-with-the-file change.As per coding guidelines, "Never use
.unwrap(),.expect(),panic!(), orassert!()in library code for fallible operations; use?withResultand proper error types."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-encoding/src/encodings/logical/primitive.rs` around lines 1793 - 1855, Replace the file-derived assert in build_chunk_index with structured invalid-input error handling: make build_chunk_index return Result<MiniBlockChunkIndex, Error>, propagate the result through its callers with ?, and return Error::invalid_input_source when rep_index_data is not 8-byte aligned. Preserve successful chunk-index construction for valid repetition-index data and update the final MiniBlockChunkIndex::new path accordingly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rust/lance-encoding/src/encodings/logical/primitive.rs`:
- Around line 1744-1787: Make analyze_value_counts and flat_value_counts_iter
fallible when subtracting counted values from items_in_page. Replace unchecked
subtraction with checked subtraction and return a descriptive
Error::invalid_input_source when decoded metadata exceeds the page count, rather
than relying on debug_assert! or wrapping. Propagate the error through
build_chunk_index and its existing Result-returning callers.
- Around line 1793-1855: Replace the file-derived assert in build_chunk_index
with structured invalid-input error handling: make build_chunk_index return
Result<MiniBlockChunkIndex, Error>, propagate the result through its callers
with ?, and return Error::invalid_input_source when rep_index_data is not 8-byte
aligned. Preserve successful chunk-index construction for valid repetition-index
data and update the final MiniBlockChunkIndex::new path accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 502d411f-b9a4-49ae-bba1-cc2258f30e29
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlrust/lance-encoding/src/encodings/logical/primitive.rsrust/lance-file/src/reader.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/registry.rsrust/lance/src/index.rsrust/lance/src/index/vector/ivf/v2.rs
1 participant
Summary
InternalCacheKey.CacheBackendobject-safe while simplifying it around one physical key type; preserve codecs, accounting,clear, and Moka single-flight behavior.Closes #7832.
Stable key format
CACHE_KEY_FORMATisblake3-128-v1:derive_keyusing a fixed Lance context.with_key_prefixsegment derives a new keyed namespace with explicit domain and length framing.#[repr(u8)]enum. Variable-width values are length-framed; integers are fixed-width little-endian; options, variants, sequences, fixed bytes, and variable bytes have distinct one-byte tags.InternalCacheKey::{as_bytes,into_bytes,from_bytes}are the persistence boundary.Changing a key schema version intentionally produces a cold miss. Persistent backends should include
CACHE_KEY_FORMATin their physical namespace and allow entries from older formats to age out; there is no runtime legacy-key fallback.The digest is a cache identity, not an authentication or authorization mechanism. Deterministic namespace keys are not secret. A 128-bit digest has approximately 64 bits of generic birthday-collision resistance and 128 bits of targeted preimage resistance. This change does not add a FIPS mode: #7832 selects BLAKE3 and Lance has no existing FIPS configuration surface.
Intentional backend API break
This removes APIs that require retaining logical strings or a secondary inventory:
CacheBackend::invalidate_prefixLanceCache::keysUse
clearfor explicit invalidation, or derive/version a new namespace when a logical scope changes.Custom backend migration:
InternalCacheKeydirectly, or persistkey.into_bytes()as exactly 16 bytes.InternalCacheKey::from_byteswhen needed.CacheCodec::type_id()instead of inferring value type from a logical key string.with_backend_and_prefixwithwith_backend(...).with_key_prefix(...).CacheBackendtrait remains non-generic and object-safe.Existing out-of-tree
CacheKey/UnsizedCacheKeyimplementations retain a source-compatible default bridge throughkey(). Performance-sensitive implementations should define a stableCacheKeySchemaand overridewrite_key; all current in-tree producers do so.Correctness and persistence coverage
The proof suite covers:
The removed unstable IVF
cache_key_prefixprotobuf payload is reserved by field number and name. Its codec version is unchanged because protobuf removal is wire-compatible and the new physical-key format already guarantees a cold miss.Benchmarks
Three independent Criterion passes used
release-with-debug, 100 samples, Rust 1.97.0, and an AMD Ryzen 9 3900X under x86_64 WSL2. Both paths include the backend's outer hash. Reported ranges compare median time for the fixed path against the benchmark-local implementation of the previous string-key path:Namespace derivation is benchmarked separately so one-time scope setup is not folded into per-entry preparation.
Validation
Base:
a3c6fce816befb7072505fbe05cc55cd205a171ePassed after rebasing onto that base and again after review follow-ups:
cargo fmt --all -- --checkCARGO_INCREMENTAL=0 cargo check --workspace --tests --benches --lockedCARGO_INCREMENTAL=0 cargo clippy --all --tests --benches -- -D warningsCARGO_INCREMENTAL=0 cargo test --workspace --lockedCARGO_INCREMENTAL=0 cargo +1.91.0 check --workspace --tests --benches --lockedCARGO_INCREMENTAL=0 cargo check --manifest-path python/Cargo.toml --lockedCARGO_INCREMENTAL=0 cargo check --manifest-path java/lance-jni/Cargo.toml --lockedAll three lockfiles contain only the intentional BLAKE3 dependency change (plus the upstream release-version updates already present in the base).
Interaction with open cache work
Made with Cursor