fix(index): replace scalar cache entries on store rotation - #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
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
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
878987c to
58d1ac3
Compare
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
7c0e630 to
f3af492
Compare
u70b3
changed the title
048f7e0 to
e5921b6
Compare
|
Rebased the unchanged one-commit binding-replacement patch onto current main (new head e5921b6) to regenerate CI artifacts. The previous linux-build failure was infrastructure-only: nextest could not extract the coverage archive because the zstd stream ended with incomplete frame; all other substantive Rust, Python, Java, and compatibility jobs passed. Fresh isolated local validation passed cargo fmt, both targeted store-rotation/prewarm regressions, and full-workspace clippy. Please re-review the minimized patch. @coderabbitai review |
e5921b6 to
7691a7a
Compare
5f68074 to
ff0a8b4
Compare
ff0a8b4 to
db366a4
Compare
|
@Xuanwo could you take another look when you have a chance? I rebased this onto current Local validation passed:
|
f3dc75e to
bced32f
Compare
Live scalar cache entries retained readers from the ObjectStore used on first open. Cache each live index together with its storage binding, reuse it only for an equivalent binding, and overwrite it on rotation. Portable plugin state remains unchanged. Update SingleScalarContainerCacheBackend to also reject store-bound cache entries so the partial-prewarm tests from lance-format#8298 keep simulating a single-container cache.
bced32f to
022cca8
Compare
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-changes
Keep one stable scalar cache slot across store rotations and recheck the binding under a shared async guard so concurrent credential-generation opens reuse a single reload.
lance-gatekeeper
Bot
removed
the
K-changes
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The stable per-index cache slot now serializes store-binding replacement and rechecks beneath the shared async guard, so equivalent concurrent rotations perform one reload while atomically switching readers to the current object store. The targeted current-store, prewarm, and eight-way concurrent cold-open regressions pass.
Please mark this PR with the breaking-change label.
lance-gatekeeper
Bot
added
the
K-approved
1 participant
Summary
Scope
This PR fixes only store-bound live scalar cache entries. It does not add TTL/TTI, change vector or legacy-index behavior, or introduce a new portable FTS state format. #7958 described an intermediate #7944 design that was not merged and is being corrected separately.
Custom IndexStore implementations are conservative by default: they do not reuse a live index unless they explicitly establish storage-binding equivalence.
Testing
Closes #7959