fix(index): avoid caching store-bound scalar indices - #7962
Conversation
github-actions
Bot
added
A-index
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIndex caching now avoids retaining store-bound scalar and vector objects across object-store generations. Portable IVF state remains cacheable, while readers are reopened against the current store. Regression tests verify cache isolation, current-store IO, and prewarm behavior. ChangesIndex cache isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dataset
participant IndexCache
participant ObjectStore
participant VectorIndex
Dataset->>IndexCache: open index
IndexCache-->>Dataset: portable IVF state or cache miss
Dataset->>ObjectStore: open readers for current store
ObjectStore-->>VectorIndex: index data
VectorIndex-->>Dataset: reconstructed index
Dataset->>IndexCache: store portable IVF state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
f1a2cca to
f6fdf31
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust/lance/src/index/vector/ivf/v2.rs (1)
6570-6585: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
#[cfg_attr(coverage, coverage(off))]to this test utility.
NoPartitionCacheBackendand itsCacheBackendimpl are test-only scaffolding and should be excluded from coverage instrumentation.♻️ Suggested annotations
+#[cfg_attr(coverage, coverage(off))] #[derive(Debug)] struct NoPartitionCacheBackend(lance_core::cache::MokaCacheBackend);#[async_trait::async_trait] +#[cfg_attr(coverage, coverage(off))] impl lance_core::cache::CacheBackend for NoPartitionCacheBackend {As per coding guidelines: "disable coverage for test utilities with
#[cfg_attr(coverage, coverage(off))]".🤖 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/src/index/vector/ivf/v2.rs` around lines 6570 - 6585, Add #[cfg_attr(coverage, coverage(off))] to the NoPartitionCacheBackend test utility and its CacheBackend implementation, excluding both scaffolding components from coverage instrumentation.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.
Inline comments:
In `@rust/lance-index-core/src/scalar.rs`:
- Around line 249-257: Expand the documentation for
can_cache_store_bound_indices with a compiling usage example that demonstrates
both true and false implementations, and link to the relevant cache and
index-opening APIs using their actual symbols and signatures. Keep the example
synchronized with the public contract and clarify when implementers should
override the default.
In `@rust/lance/src/index.rs`:
- Around line 2942-2975: Replace the manually constructed test fixtures in the
batch setup around RecordBatchIterator with arrow_array’s record_batch!() macro.
Remove the redundant Schema, Arc, and RecordBatch::try_new boilerplate while
preserving the existing id and text values and resulting RecordBatchIterator
behavior.
---
Nitpick comments:
In `@rust/lance/src/index/vector/ivf/v2.rs`:
- Around line 6570-6585: Add #[cfg_attr(coverage, coverage(off))] to the
NoPartitionCacheBackend test utility and its CacheBackend implementation,
excluding both scaffolding components from coverage instrumentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 6335ac2e-99de-4a65-815f-d9354fafa37f
📒 Files selected for processing (5)
rust/lance-index-core/src/scalar.rsrust/lance-index/src/scalar/lance_format.rsrust/lance-index/src/scalar/registry.rsrust/lance/src/index.rsrust/lance/src/index/vector/ivf/v2.rs
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 (1)
rust/lance/src/index.rs (1)
2400-2557: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
IVF_RQopens against the wrong directory.Every other IVF variant branch (
IVF_FLAT,IVF_PQ,IVF_SQ,IVF_HNSW_FLAT,IVF_HNSW_SQ,IVF_HNSW_PQ) passes the locally-resolvedindex_dir(fromself.indice_files_dir(&index_meta), which honorsindex_meta.base_idfor e.g. shallow-cloned datasets).IVF_RQinstead passesself.indices_dir(), ignoring any base redirection. For anIVF_RQindex whose metadata points at a different base (cloned dataset), this will resolve to the wrong location and fail to open.🐛 Proposed fix
"IVF_RQ" => { let ivf = IVFIndex::<FlatIndex, RabitQuantizer>::try_new( object_store.clone(), - self.indices_dir(), + index_dir, uuid.to_owned(), frag_reuse_index, self.metadata_cache.as_ref(), index_cache, file_sizes, ) .await?; Ok(wrap_ivf(ivf)) }🤖 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/src/index.rs` around lines 2400 - 2557, Update the IVF_RQ branch in the index-opening match to pass the locally resolved index_dir to IVFIndex::try_new, matching every other IVF variant. Do not use self.indices_dir(), so index metadata base redirection remains honored.
🤖 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/src/index.rs`:
- Around line 2400-2557: Update the IVF_RQ branch in the index-opening match to
pass the locally resolved index_dir to IVFIndex::try_new, matching every other
IVF variant. Do not use self.indices_dir(), so index metadata base redirection
remains honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 5c7e1498-9d16-4f7a-9f13-3d6df6327c64
📒 Files selected for processing (1)
rust/lance/src/index.rs
9404ed7 to
e140b65
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance/src/index.rs (1)
2515-2527: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass
index_dirtoIVF_RQconstruction
IVF_RQcurrently constructs withself.indices_dir(), which always uses the dataset root and ignores the index metadata’sbase_id. Like the other IVF branches, use the already resolvedindex_dirfromindice_files_dir(&index_meta).🐛 Proposed fix
"IVF_RQ" => { let ivf = IVFIndex::<FlatIndex, RabitQuantizer>::try_new( object_store.clone(), - self.indices_dir(), + index_dir, uuid.to_owned(),🤖 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/src/index.rs` around lines 2515 - 2527, Update the IVF_RQ branch’s IVFIndex::<FlatIndex, RabitQuantizer>::try_new call to pass the already resolved index_dir from indice_files_dir(&index_meta) instead of self.indices_dir(). Keep the remaining construction arguments and wrap_ivf flow unchanged.
🟡 Other comments (2)
rust/lance/src/index.rs-3277-3287 (1)
3277-3287: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis assertion passes vacuously if the cache key type name ever changes.
The regression check filters on the literal
"ScalarIndex". If that type name is renamed, the filter matches nothing,scalar_cache_entriesis empty, and the assertion succeeds while store-bound scalar indices are once again being retained — the exact regression this test exists to catch.Assert the filter is meaningful, e.g. capture all key type names and additionally assert that the expected FTS/scalar-related name is absent, or derive the literal from the key type rather than hard-coding it.
🤖 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/src/index.rs` around lines 3277 - 3287, Update the cache assertion in the test around index_cache_keys so it cannot pass vacuously when the scalar index key type name changes. Replace the hard-coded "ScalarIndex" filter with a type-name value derived from the relevant key type, or separately validate that the expected scalar/FTS key name exists before asserting no matching entries remain.rust/lance/src/index.rs-1629-1635 (1)
1629-1635: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winError message omits which segment failed and which condition tripped.
With a multi-segment batch the caller cannot tell whether the problem is retired fragment coverage or a fragment-reuse mapping, nor which segment uuid to rebuild. Include the offending uuid(s) and the triggering condition.
As per coding guidelines, "Include full context in error messages such as variable names, values, sizes, types, and indices; avoid generic messages."
🛠️ Sketch
- if has_retired_coverage || requires_rebuild { - return Err(Error::invalid_input( - "CreateIndex: NGram segments built before compaction must be rebuilt or merged \ - with merge_existing_index_segments before commit" - .to_string(), - )); - } + if has_retired_coverage || requires_rebuild { + let reason = if has_retired_coverage { + "cover fragments no longer in the dataset" + } else { + "are affected by a fragment-reuse mapping" + }; + let uuids = segments + .iter() + .map(|segment| segment.uuid().to_string()) + .collect::<Vec<_>>() + .join(", "); + return Err(Error::invalid_input(format!( + "CreateIndex: NGram segments [{uuids}] {reason}; rebuild them or merge with \ + merge_existing_index_segments before commit" + ))); + }🤖 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/src/index.rs` around lines 1629 - 1635, Update the invalid-input error in the has_retired_coverage/requires_rebuild branch to include the offending segment UUID(s) and identify whether retired coverage or fragment-reuse mapping triggered the failure. Use the relevant variables available in the surrounding index-creation flow, while preserving the existing rebuild-or-merge guidance.Source: Coding guidelines
🧹 Nitpick comments (3)
rust/lance/src/index.rs (1)
1586-1588: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplain why NGram and FMIndex opt out of the
merged_dataset_versionoverwrite.
ngram::merge_segmentsalready setsdataset_versionitself (current manifest version on the rebuild path,source_dataset_versionotherwise), so overwriting it here would mislabel a rebuilt segment as stale. That reasoning is invisible at this call site, and the negative list will rot as other merge paths start owning their own version.As per coding guidelines, "Comment fallback or guard code paths with when they trigger and why they exist."
♻️ Suggested comment
+ // NGram and FMIndex merges set `dataset_version` themselves (an NGram + // rebuild is stamped with the current manifest version), so overwriting + // it here would mark a freshly rebuilt segment as stale. if !all_ngram && !all_fmindex { merged_segment.dataset_version = merged_dataset_version; }🤖 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/src/index.rs` around lines 1586 - 1588, Add an explanatory comment alongside the `if !all_ngram && !all_fmindex` guard in the merge flow, stating that NGram and FMIndex merge paths assign `dataset_version` themselves and must not be overwritten because rebuilt segments require the current manifest version while other paths may use `source_dataset_version`. Keep the existing condition and assignment unchanged.Source: Coding guidelines
rust/lance/src/index/vector/ivf/v2.rs (2)
6739-6741: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse named rstest cases.
♻️ Proposed rename
#[rstest] -#[case(IndexFileVersion::V3)] -#[case(IndexFileVersion::Legacy)] +#[case::v3(IndexFileVersion::V3)] +#[case::legacy(IndexFileVersion::Legacy)] #[tokio::test]As per coding guidelines: "Use
rstestfor Rust parameterized tests, use readable#[case::{name}(...)]names".🤖 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/src/index/vector/ivf/v2.rs` around lines 6739 - 6741, Update the parameterized test cases in the visible rstest declaration to use readable named cases, such as case names identifying IndexFileVersion::V3 and IndexFileVersion::Legacy, while preserving their existing values and test coverage.Source: Coding guidelines
6587-6589: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType-name string matching can silently stop intercepting partition entries.
is_partitiondepends on the exacttype_name()of the IVF partition cache keys. If those names change, this backend silently caches partitions again andtest_vector_cache_uses_current_object_storedegrades into a vacuous pass (no per-query partition reads, so the store-routing assertions lose their teeth). Consider tracking intercepted keys in the backend and asserting a non-zero count in the test, so a rename fails loudly.🤖 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/src/index/vector/ivf/v2.rs` around lines 6587 - 6589, Update the backend cache interception around is_partition to track how many partition-entry keys are intercepted, and expose that count for test verification. In test_vector_cache_uses_current_object_store, assert the count is non-zero so renamed type names fail the test instead of making it vacuously pass; preserve the existing routing assertions.
🤖 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/src/index.rs`:
- Around line 2515-2527: Update the IVF_RQ branch’s IVFIndex::<FlatIndex,
RabitQuantizer>::try_new call to pass the already resolved index_dir from
indice_files_dir(&index_meta) instead of self.indices_dir(). Keep the remaining
construction arguments and wrap_ivf flow unchanged.
---
Other comments:
In `@rust/lance/src/index.rs`:
- Around line 3277-3287: Update the cache assertion in the test around
index_cache_keys so it cannot pass vacuously when the scalar index key type name
changes. Replace the hard-coded "ScalarIndex" filter with a type-name value
derived from the relevant key type, or separately validate that the expected
scalar/FTS key name exists before asserting no matching entries remain.
- Around line 1629-1635: Update the invalid-input error in the
has_retired_coverage/requires_rebuild branch to include the offending segment
UUID(s) and identify whether retired coverage or fragment-reuse mapping
triggered the failure. Use the relevant variables available in the surrounding
index-creation flow, while preserving the existing rebuild-or-merge guidance.
---
Nitpick comments:
In `@rust/lance/src/index.rs`:
- Around line 1586-1588: Add an explanatory comment alongside the `if !all_ngram
&& !all_fmindex` guard in the merge flow, stating that NGram and FMIndex merge
paths assign `dataset_version` themselves and must not be overwritten because
rebuilt segments require the current manifest version while other paths may use
`source_dataset_version`. Keep the existing condition and assignment unchanged.
In `@rust/lance/src/index/vector/ivf/v2.rs`:
- Around line 6739-6741: Update the parameterized test cases in the visible
rstest declaration to use readable named cases, such as case names identifying
IndexFileVersion::V3 and IndexFileVersion::Legacy, while preserving their
existing values and test coverage.
- Around line 6587-6589: Update the backend cache interception around
is_partition to track how many partition-entry keys are intercepted, and expose
that count for test verification. In
test_vector_cache_uses_current_object_store, assert the count is non-zero so
renamed type names fail the test instead of making it vacuously pass; preserve
the existing routing assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 2e098483-deaa-4fde-a040-d7b82d187463
📒 Files selected for processing (5)
rust/lance-index-core/src/scalar.rsrust/lance-index/src/scalar/lance_format.rsrust/lance-index/src/scalar/registry.rsrust/lance/src/index.rsrust/lance/src/index/vector/ivf/v2.rs
Store-bound live indices must reopen against the current object store, while prewarm assertions verify reuse of portable entries. Restore the dependency fixes lost during rebase, including IVF_RQ base-path handling.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance/src/index.rs (1)
3139-3139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
memory://for this test, or document why a filesystem store is required.This test creates a temporary directory despite the repository rule requiring plain
memory://URIs. Verify that the in-memory backend supports the configured credential accessor and I/O tracking; if so, switch the fixture tomemory://.As per coding guidelines, “Use plain
"memory://"URIs in tests; no atomic counters or unique suffixes are needed.”🤖 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/src/index.rs` at line 3139, Update the test fixture around TempStrDir::default to use the plain memory:// URI instead of a temporary filesystem directory, after verifying the in-memory backend supports the configured credential accessor and I/O tracking; if it does not, retain the filesystem store and document that requirement in the test.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/src/index.rs`:
- Line 3139: Update the test fixture around TempStrDir::default to use the plain
memory:// URI instead of a temporary filesystem directory, after verifying the
in-memory backend supports the configured credential accessor and I/O tracking;
if it does not, retain the filesystem store and document that requirement in the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 91de701e-f553-4d62-9ea3-2b4b28d2fc8e
📒 Files selected for processing (6)
python/python/tests/test_scalar_index.pyrust/lance-index-core/src/scalar.rsrust/lance/src/dataset/tests/dataset_index.rsrust/lance/src/index.rsrust/lance/src/index/vector/ivf.rsrust/lance/src/index/vector/ivf/v2.rs
Coverage helpers must annotate executable impl blocks, and the lance crate needs the nightly coverage_attribute gate when built by llvm-cov.
Credential-specific memory stores do not share index data, so the store-rotation test needs a filesystem backend to reopen the same index through distinct wrappers.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Summary
IndexStoreimplementations declare whether live indices holding their readers are safe to share through a Session cacheScalarIndexcache forLanceIndexStore, so FTS and other default scalar plugins always bind readers to the current object storeLifecycle
Depends on #7944. Together, the two PRs remove the known store-bound vector and scalar entries from Session caches. This resolves stale-generation retention deterministically without adding TTL/TTI or process-local object-store identities.
Testing
cargo fmt --all -- --checkcargo check -p lance-index-core -p lance-index -p lance --libcargo test -p lance --lib test_scalar_cache_uses_current_object_store -- --nocapturecargo test -p lance-index test_btree_index_state_reconstruct_and_plugin_cachecargo test -p lance-index test_bitmap_cache_fast_pathcargo clippy --all --tests --benches -- -D warningsThe current #7944 base contains an unrelated blob merge-insert test compile error (
Option<BlobFile>::read). The scalar regression and full clippy were run with a local-only six-line unwrap fix for that base error; that workaround is not included here.Closes #7958
Closes #7959