fix(index): rebind vector readers to current object store#7944
Conversation
|
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:
📝 WalkthroughWalkthroughVector index caching now accounts for object-store identity. Legacy live-index caching is removed, IVF partition namespaces use IVF state keys, and reconstructed IVF readers bind to the current object store while reusable metadata remains cached. ChangesObject-store cache identity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dataset
participant IVFIndex
participant MetadataCache
participant ObjectStore
Dataset->>IVFIndex: reconstruct_typed(object_store)
IVFIndex->>MetadataCache: reuse portable file metadata
IVFIndex->>ObjectStore: open readers for current store
ObjectStore-->>IVFIndex: return current-store readers
IVFIndex-->>Dataset: return reconstructed index
Possibly related issues
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 |
d65ca56 to
b649e77
Compare
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
This seems like the wrong solution to me. Keep in mind, some cache backends might spill these entries to disk, and reload them on restart. Or others might serialize them to another service like redis and share the entries amongst servers. So think an in-process atomic counter is too granular and could even create issues for the re-use on restart case. See my comment in this thread:
|
Thanks, I understand the intended invariant now: cached index state should be portable, and the current object store should be injected during reconstruction rather than represented in the cache key. The runtime identity in this PR is currently used only for codec-less, in-memory entries ( I will rework the V2+ path to cache only portable IVF state/file metadata and always construct readers from the current object store during reconstruction. For legacy indices, since the cached value is inherently store-bound, I will bypass the shared live-index cache rather than add store identity to its key. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
d77ffca to
dd3204f
Compare
u70b3
changed the title
|
Updated in
The targeted rotation, prewarm, serializing-backend tests, full fmt, and full clippy all pass. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
2319-2331: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIVF_RQ passes the default indices directory while using the index-specific object store.
IVF_RQresolves a base-awareindex_dirviaindice_files_dir(&index_meta)in the same arm, but still passesself.indices_dir()intoIVFIndex::try_new. For relocated index base paths, that stores IVF partition lookup paths under the dataset default while the store prefix is already bound to the index-specific path. Passindex_dirconsistently with the other IVF arms.🐛 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 2319 - 2331, Update the IVF_RQ arm’s IVFIndex::try_new call to pass the already-resolved index_dir from indice_files_dir(&index_meta) instead of self.indices_dir(), keeping the index-specific object store and directory prefix consistent for relocated index bases.
🤖 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 2319-2331: Update the IVF_RQ arm’s IVFIndex::try_new call to pass
the already-resolved index_dir from indice_files_dir(&index_meta) instead of
self.indices_dir(), keeping the index-specific object store and directory prefix
consistent for relocated index bases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: f1c3b8b7-e1f0-4ea4-9eae-d9f90c4d3eb4
📒 Files selected for processing (2)
rust/lance/src/index.rsrust/lance/src/index/vector/ivf/v2.rs
dd3204f to
91436c2
Compare
91436c2 to
1e9cc8a
Compare
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)
2319-2331: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the resolved index directory for IVF_RQ. This arm passes
self.indices_dir()while the other v0.3+ arms passindex_dir, which is already resolved byself.indice_files_dir(&index_meta). For indices stored on external/base paths, IVF_RQ readers can be opened from the wrong location; passindex_dirtoIVFIndex::<FlatIndex, RabitQuantizer>::try_new(...).🤖 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 2319 - 2331, Update the IVF_RQ arm in the index-loading match to pass the resolved index_dir from self.indice_files_dir(&index_meta) into IVFIndex::<FlatIndex, RabitQuantizer>::try_new instead of self.indices_dir(), matching the other v0.3+ arms and preserving external/base-path resolution.
🤖 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 2319-2331: Update the IVF_RQ arm in the index-loading match to
pass the resolved index_dir from self.indice_files_dir(&index_meta) into
IVFIndex::<FlatIndex, RabitQuantizer>::try_new instead of self.indices_dir(),
matching the other v0.3+ arms and preserving external/base-path resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: a42bbf26-1e3b-4d4e-bcd6-17426fd6be3f
📒 Files selected for processing (2)
rust/lance/src/index.rsrust/lance/src/index/vector/ivf/v2.rs
|
Updated the PR and synchronized it with current main. Follow-up changes:
Validation:
The previous runtime-identity implementation is no longer present; the PR now caches only store-free IVF state/metadata/partitions and reconstructs readers with the current object store. @wjones127, could you please re-review the portable-state version? |
|
Follow-up for the first CI run:
The previously failing legacy single-delta and multi-delta tests now pass locally, along with the credential-rotation and IVF_RQ shallow-clone regressions. Full fmt and workspace Clippy also pass. Pushed as 9543f79. |
2 participants
Summary
Testing
cargo fmt --all -- --checkcargo clippy --all --tests --benches -- -D warningscargo test -p lance --lib test_vector_cache_uses_current_object_store -- --nocapturecargo test -p lance --lib test_prewarm_ivf_pq -- --nocapturecargo test -p lance --lib test_prewarm_and_query_with_serializing_backend -- --nocaptureCloses #7904