feat: prototype xabi cache backend ABI - #7873
Conversation
📝 WalkthroughWalkthroughAdds an XABI contract for dynamically loaded cache backends, a Lance adapter with fallback caching, a cdylib fixture, ABI snapshots, and tests covering backend operations, typed errors, cache routing, and indexed-query prewarming. ChangesDynamic cache backend
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant DynamicCacheBackendAdapter
participant DynamicCacheBackend
participant CacheCodec
participant ObjectStore
Session->>DynamicCacheBackendAdapter: request indexed cache entry
DynamicCacheBackendAdapter->>DynamicCacheBackend: lookup serialized key
DynamicCacheBackend-->>DynamicCacheBackendAdapter: bytes or cache miss
DynamicCacheBackendAdapter->>CacheCodec: decode cached bytes
CacheCodec-->>DynamicCacheBackendAdapter: typed entry
DynamicCacheBackendAdapter-->>Session: return cache entry
Session->>ObjectStore: read only on cache miss
Possibly related PRs
🚥 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 |
a24806a to
1c23359
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
372a5f1 to
6a5250d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance-core/src/cache/dynamic.rs-23-24 (1)
23-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument these magic hash-seed constants.
KEY_HASH_SEED_0/KEY_HASH_SEED_1have no comment explaining what they encode or why these specific values were chosen (they decode to ASCII strings, presumably intentional domain separation constants).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."
📝 Proposed doc comments
+/// Domain-separation seed for the high 64 bits of the plugin cache key +/// (arbitrary ASCII-derived constant, chosen only to differ from `KEY_HASH_SEED_1`). const KEY_HASH_SEED_0: u64 = 0x4c41_4e43_455f_4341; +/// Domain-separation seed for the low 64 bits of the plugin cache key. const KEY_HASH_SEED_1: u64 = 0x4348_455f_4142_4931;🤖 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-core/src/cache/dynamic.rs` around lines 23 - 24, Add documentation comments for KEY_HASH_SEED_0 and KEY_HASH_SEED_1 explaining that the hexadecimal values encode intentional ASCII domain-separation strings and why these specific seeds are used for key hashing.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-cache-abi/src/xabi_contract.rs`:
- Around line 4-14: Update the public documentation in xabi_contract.rs for
CacheKey128, DynamicCacheBackend, CacheLookup, CacheMeasure, CacheAbiError, the
public handle aliases, and loader functions to include compile-tested examples
and intra-API links to related structs and methods. Keep examples synchronized
with the current ABI behavior and document DYNAMIC_CACHE_BACKEND_TRAIT_ID if it
is part of the public surface.
- Around line 108-130: Document and enforce the concurrency contract for
DynamicCacheBackend: require implementations to be safe for concurrent
shared-handle calls by adding the appropriate Send and Sync bounds to the trait,
and state in its documentation that get, insert, clear, and measure may run
concurrently through CacheBackend paths. Do not introduce adapter-side
serialization unless the existing ABI design already requires it.
In `@rust/lance-cache-abi/tests/xabi_dynamic_backend.rs`:
- Around line 61-102: Extract fixture_library_path, build_fixture_library, and
dynamic_library_name from rust/lance-cache-abi/tests/xabi_dynamic_backend.rs
lines 61-102 into a shared dev-dependencies-only test-support crate under
rust/lance-cache-abi, adding #[cfg_attr(coverage, coverage(off))] to each
function. Update rust/lance-core/src/cache/dynamic.rs lines 404-445 to use the
shared helper instead of its duplicated implementation, and replace the
dynamic_cache_fixture module in rust/lance/src/dataset/tests/dataset_index.rs
lines 2593-2643 with the same helper; preserve the existing fixture build and
platform-specific library path behavior at all sites.
In `@rust/lance-core/src/cache/dynamic.rs`:
- Around line 198-207: The invalidate_prefix method currently clears the entire
dynamic backend, evicting entries unrelated to the requested prefix. Update
DynamicCacheBackendAdapter::invalidate_prefix to invalidate only entries
belonging to prefix, preserving cached entries for other datasets or keys;
remove the unconditional backend.clear path and keep the counters consistent
with the entries actually removed.
In `@rust/lance-core/src/cache/mod.rs`:
- Around line 59-61: Replace the broad `lance_cache_abi` re-export with a scoped
`dynamic_cache_abi` module or equivalent re-export exposing only `CacheKey128`
and `DynamicCacheBackendHandle`, which are required by `dynamic.rs`; keep those
symbols accessible where needed without exposing the ABI crate’s remaining
public items through `lance-core`.
---
Other comments:
In `@rust/lance-core/src/cache/dynamic.rs`:
- Around line 23-24: Add documentation comments for KEY_HASH_SEED_0 and
KEY_HASH_SEED_1 explaining that the hexadecimal values encode intentional ASCII
domain-separation strings and why these specific seeds are used for key hashing.
🪄 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: 1e31d3fd-89ae-4c3f-88b9-332321bb4181
⛔ 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 (18)
Cargo.tomlrust/lance-cache-abi/Cargo.tomlrust/lance-cache-abi/src/lib.rsrust/lance-cache-abi/src/xabi_contract.rsrust/lance-cache-abi/tests/fixtures/xabi-cache-plugin/Cargo.tomlrust/lance-cache-abi/tests/fixtures/xabi-cache-plugin/src/lib.rsrust/lance-cache-abi/tests/xabi_dynamic_backend.rsrust/lance-cache-abi/xabi/snapshots/org.lance.cache.DynamicCacheBackend/aarch64-apple-darwin.txtrust/lance-cache-abi/xabi/snapshots/org.lance.cache.DynamicCacheBackend/aarch64-unknown-linux-gnu.txtrust/lance-cache-abi/xabi/snapshots/org.lance.cache.DynamicCacheBackend/x86_64-apple-darwin.txtrust/lance-cache-abi/xabi/snapshots/org.lance.cache.DynamicCacheBackend/x86_64-pc-windows-gnu.txtrust/lance-cache-abi/xabi/snapshots/org.lance.cache.DynamicCacheBackend/x86_64-pc-windows-msvc.txtrust/lance-cache-abi/xabi/snapshots/org.lance.cache.DynamicCacheBackend/x86_64-unknown-linux-gnu.txtrust/lance-core/Cargo.tomlrust/lance-core/src/cache/dynamic.rsrust/lance-core/src/cache/mod.rsrust/lance-core/src/cache/moka.rsrust/lance/src/dataset/tests/dataset_index.rs
| //! Experimental xabi contract for dynamically loaded Lance cache backends. | ||
| //! | ||
| //! This prototype keeps the host-side cache policy out of the low-level ABI: | ||
| //! Lance remains responsible for typed cache entries, `codec == None` fallback, | ||
| //! singleflight, metrics, registration, and lifecycle policy. Dynamic backends | ||
| //! only receive an async Rust-shaped contract for serialized cache bytes. | ||
|
|
||
| use std::path::Path; | ||
|
|
||
| /// Stable xabi contract identifier for Lance cache backends. | ||
| pub const DYNAMIC_CACHE_BACKEND_TRAIT_ID: &str = "org.lance.cache.DynamicCacheBackend"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add examples and intra-API links to the public ABI documentation.
The new public contract types, trait, handle aliases, and loader functions have prose docs but no compile-tested examples or links to related APIs. Add synchronized examples and links for the public surface, including CacheKey128, DynamicCacheBackend, CacheLookup, CacheMeasure, CacheAbiError, and the loader functions.
As per coding guidelines, **/*.{rs,md,rst} requires documenting all public APIs with examples and links to relevant structs and methods.
Also applies to: 16-75, 100-168
🤖 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-cache-abi/src/xabi_contract.rs` around lines 4 - 14, Update the
public documentation in xabi_contract.rs for CacheKey128, DynamicCacheBackend,
CacheLookup, CacheMeasure, CacheAbiError, the public handle aliases, and loader
functions to include compile-tested examples and intra-API links to related
structs and methods. Keep examples synchronized with the current ABI behavior
and document DYNAMIC_CACHE_BACKEND_TRAIT_ID if it is part of the public surface.
Source: Coding guidelines
| pub trait DynamicCacheBackend { | ||
| /// Human-readable backend implementation name. | ||
| fn name(&self) -> String; | ||
|
|
||
| /// Look up serialized bytes by key. | ||
| async fn get( | ||
| &self, | ||
| key: CacheKey128, | ||
| ) -> std::result::Result<Option<CacheLookup>, CacheAbiError>; | ||
|
|
||
| /// Insert serialized bytes by key. | ||
| async fn insert( | ||
| &self, | ||
| key: CacheKey128, | ||
| value: &[u8], | ||
| size_bytes: usize, | ||
| ) -> std::result::Result<(), CacheAbiError>; | ||
|
|
||
| /// Clear all entries owned by this backend instance. | ||
| async fn clear(&self) -> std::result::Result<(), CacheAbiError>; | ||
|
|
||
| /// Return approximate backend size metrics. | ||
| async fn measure(&self) -> std::result::Result<CacheMeasure, CacheAbiError>; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 '\bDynamicCacheBackendHandle\b|\bbackend\.(get|insert|clear|measure)\(' \
rust/lance-core rust/lance-cache-abi
rg -n -C3 'unsafe impl.*(Send|Sync)|impl.*(Send|Sync)' \
rust/lance-cache-abiRepository: lance-format/lance
Length of output: 14142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lance-cache-abi files =="
git ls-files rust/lance-cache-abi | sort
echo "== xabi_contract.rs relevant section =="
sed -n '1,180p' rust/lance-cache-abi/src/xabi_contract.rs | cat -n
echo "== dynamic.rs adapter relevant section =="
sed -n '1,260p' rust/lance-core/src/cache/dynamic.rs | cat -n
echo "== xabi crate dependency =="
rg -n 'xabi|lance-cache-abi' Cargo.lock Cargo.toml rust/lance-core/Cargo.toml rust/lance-cache-abi/Cargo.toml
echo "== Send/Sync impls/search in xabi-related crates =="
rg -n -C3 'unsafe .*Sync|impl .*Sync|unsafe .*Send|impl .*Send|xabi::|XabiV1HandleTraitDynamicCacheBackend|trait DynamicCacheBackend' Cargo.lock rust/lance-cache-abi rust/lance-core/src/cacheRepository: lance-format/lance
Length of output: 50374
🌐 Web query:
xabi Rust crate XabiV1HandleTraitDynamicCacheBackend Send Sync xabi::xabi trait generated handle
💡 Result:
The xabi crate is a Rust library designed to generate a stable, C-compatible Application Binary Interface (ABI) from Rust traits [1][2]. Regarding the naming convention you noted, xabi generates various ABI-related artifacts using an explicit XabiV1 prefix [1][3]. These generated types, such as XabiV1HandleTrait, are internal ABI artifacts [1][2]. The crate's documentation explicitly notes that these names are part of the generated ABI and are not intended for direct use in general application code; domain crates should generally re-export only the handles or helper APIs they want users to see [1][3]. Specific details regarding the thread safety (Send/Sync) of these generated handles depend on the underlying trait definition and the specific handle type generated (e.g., owned vs. borrowed handles) [2][3]. Because xabi keeps the public contract as ordinary Rust traits [2], the Send and Sync implementations for the generated handles typically follow standard Rust auto-trait rules based on the types involved in the trait methods. If you are encountering specific issues with Send or Sync requirements for a handle, consider that: 1. The crate generates specific artifacts like XabiV1BorrowedTrait* and XabiV1OwnedTrait* [1][4]. 2. xabi handles are designed to work across dynamic module boundaries [1][3]. 3. For complex scenarios, the crate provides mechanisms such as panic guards and typed error handling to ensure ABI stability [2][3]. For verification of layout and ABI stability, the associated xabi-assert crate provides the assert_abi! macro to ensure generated contracts match committed snapshots [5][6]. You can use this to verify if your trait-generated handles maintain expected properties across versions [5]. If your specific handle name is XabiV1HandleTraitDynamicCacheBackend, it is almost certainly a type generated by the xabi macro for a trait named DynamicCacheBackend [1][2]. You should verify the trait definition for that name in your project to check if its methods or associated data types contain non-Send/Sync members [2].
Citations:
- 1: https://crates.io/crates/xabi
- 2: https://github.com/Xuanwo/xabi
- 3: https://github.com/Xuanwo/xabi/blob/main/README.md
- 4: https://lib.rs/crates/xabi
- 5: https://crates.io/crates/xabi-assert
- 6: https://docs.rs/xabi-assert/latest/xabi_assert/
🌐 Web query:
github xabi xabi 0.1.0 XabiV1HandleTrait<T> impl Send Sync
💡 Result:
In the xabi crate (version 0.1.0), XabiV1HandleTrait is a generated ABI artifact rather than a manually implemented trait [1]. The xabi crate automatically generates this trait, along with other artifacts like vtables and handle loaders, when you define and export Rust traits for use across FFI boundaries [1]. Because XabiV1HandleTrait is machine-generated code, its implementation of Send and Sync is determined by the properties of the underlying data and the code generation logic defined within the xabi crate itself, rather than by a user-provided impl block [1]. Users typically do not implement XabiV1HandleTrait directly; instead, they use the #[xabi::module] attribute on their own structures, and the crate handles the necessary trait implementations and ABI safety [1]. If you are encountering issues with Send/Sync requirements while using xabi, it is likely due to the nature of the handles being generated. Since these handles facilitate interaction with native code—which is inherently unsafe—the generated types must satisfy the safety requirements imposed by the xabi framework [1]. You should verify that the types you are passing across the ABI boundary are compatible with the thread-safety guarantees you require, as the generated ABI artifacts will inherit the Send/Sync status of their constituent parts [1].
Citations:
🌐 Web query:
gh raw github Xuanwo xabi 0.1.0 src generated.rs XabiV1HandleTrait<XabiV1ThreadSlotTrait<T> :: Send Sync
💡 Result:
The identifiers XabiV1HandleTrait and XabiV1ThreadSlotTrait, along with the associated types and trait bounds mentioned, are auto-generated ABI artifacts produced by the xabi crate [1]. Created by Xuanwo, xabi is a Rust library designed to generate stable native ABI (Application Binary Interface) glue from Rust traits, facilitating safe interaction between host applications and dynamic library plugins [1][2]. In the generated output (typically found in files such as generated.rs), xabi prefixes its ABI-specific artifacts with XabiV1 to ensure versioned, C-compatible stability [1]. Specifically: 1. XabiV1HandleTrait: This is part of the machinery for host-side handles [1]. The xabi library generates these traits to provide safe, stable interfaces for objects passing across the library boundary [1][2]. 2. XabiV1ThreadSlotTrait: These artifacts are used to manage thread-local storage or thread-bound resources within the context of the stable ABI boundary [1]. 3. Trait Bounds (Send, Sync): The combination of XabiV1ThreadSlotTrait:: Send Sync represents the enforced safety bounds on these generated structures [1][3]. These bounds ensure that types used within the ABI-stable interface satisfy the necessary thread-safety guarantees (Send and Sync) required for cross-boundary operations in a concurrent environment [1]. Because xabi is designed to abstract away the complexity of manual FFI (Foreign Function Interface), these generated traits should generally be considered implementation details [1]. Users are typically encouraged to rely on higher-level exported handles or APIs rather than directly implementing or interacting with these low-level ABI artifacts [1]. The library is actively maintained, with version 0.1.0 and subsequent releases (such as 0.1.2) continuing to iterate on the generated API shape [1][4].
Citations:
- 1: https://lib.rs/crates/xabi-macros
- 2: https://github.com/Xuanwo/xabi
- 3: https://docs.rs/glib/latest/glib/struct.ThreadHandle.html
- 4: https://github.com/Xuanwo/xabi/releases/tag/v0.1.2
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package source/version evidence for xabi =="
python3 - <<'PY'
from pathlib import Path
import re
for p in [Path('Cargo.lock'), Path('rust/lance-cache-abi/Cargo.toml')]:
text=p.read_text()
print(f'-- {p} --')
if p.name=='Cargo.lock':
for m in re.finditer(r'^name = "xabi"\n^\[package\]', text, re.M):
print(text[m.start(): text.find('\n\[', m.end())][:800])
else:
print(text)
PY
echo "== precise generated handle definitions in xabi source if vendored/cached by build artifacts =="
git ls-files | rg '(^|/)xabi/|xabi\.rs$|generated\.rs$|src/generated' || true
rg -n 'XabiV1HandleTrait<xabi::XabiV1ThreadSlotTrait<T> :: Send Sync|impl<.*XabiV1HandleTrait|XabiV1HandleTrait<|XabiV1ThreadSlotTrait|pub struct XabiV1Handle|unsafe impl.*Sync|impl.*Sync' $(git ls-files | rg '\.rs$' | rg 'xabi|cache-abi|cache') 2>/dev/null | head -120
echo "== adapter concurrency references =="
rg -n -C3 'get_or_insert\s*\(|get_or_insert_record|CacheBackend::get_or_insert|spawn_async|tokio|executor|executor::context|Executor|ExecutorContext' rust/lance-core rust/lance-cache-abiRepository: lance-format/lance
Length of output: 1397
Document the dynamic backend concurrency contract.
The host adapter can call the shared backend handle concurrently through get_or_insert and other CacheBackend paths, but this ABI does not state whether backend implementations must be thread-safe or whether xabi serializes shared-handle calls. Either enforce Send + Sync implementations and document that expectation, or serialize all shared-handle calls in the adapter and require that contract from plugins.
🤖 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-cache-abi/src/xabi_contract.rs` around lines 108 - 130, Document
and enforce the concurrency contract for DynamicCacheBackend: require
implementations to be safe for concurrent shared-handle calls by adding the
appropriate Send and Sync bounds to the trait, and state in its documentation
that get, insert, clear, and measure may run concurrently through CacheBackend
paths. Do not introduce adapter-side serialization unless the existing ABI
design already requires it.
| fn fixture_library_path() -> &'static Path { | ||
| FIXTURE_LIBRARY.get_or_init(build_fixture_library).as_path() | ||
| } | ||
|
|
||
| fn build_fixture_library() -> PathBuf { | ||
| let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | ||
| let workspace_dir = manifest_dir | ||
| .ancestors() | ||
| .nth(2) | ||
| .expect("lance-cache-abi lives under rust/") | ||
| .to_path_buf(); | ||
| let target_dir = workspace_dir.join("target").join("xabi-fixtures"); | ||
| let status = Command::new("cargo") | ||
| .args(["build", "-p", "lance-cache-xabi-fixture", "--target-dir"]) | ||
| .arg(&target_dir) | ||
| .args(["--message-format", "short"]) | ||
| .current_dir(&workspace_dir) | ||
| .env_remove("RUSTC_WRAPPER") | ||
| .env_remove("CARGO_TARGET_DIR") | ||
| .status() | ||
| .expect("cargo build can be launched"); | ||
| assert!(status.success(), "fixture cdylib build failed"); | ||
|
|
||
| let profile_dir = target_dir.join("debug"); | ||
| let library_path = profile_dir.join(dynamic_library_name("lance_cache_xabi_fixture")); | ||
| assert!( | ||
| library_path.exists(), | ||
| "fixture cdylib was not built at {}", | ||
| library_path.display() | ||
| ); | ||
| library_path | ||
| } | ||
|
|
||
| fn dynamic_library_name(stem: &str) -> String { | ||
| if cfg!(target_os = "macos") { | ||
| format!("lib{stem}.dylib") | ||
| } else if cfg!(target_os = "windows") { | ||
| format!("{stem}.dll") | ||
| } else { | ||
| format!("lib{stem}.so") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Fixture cdylib build/locate logic is triplicated across three crates. fixture_library_path, build_fixture_library, and dynamic_library_name are copy-pasted byte-for-byte in three separate files; none carry #[cfg_attr(coverage, coverage(off))] despite being pure test utilities.
rust/lance-cache-abi/tests/xabi_dynamic_backend.rs#L61-L102: extract this trio into a small shared test-support crate (e.g. adev-dependencies-only helper crate underrust/lance-cache-abi) thatlance-coreandlancetests can depend on, and add#[cfg_attr(coverage, coverage(off))]to each function.rust/lance-core/src/cache/dynamic.rs#L404-L445: replace this copy with a call into the shared helper crate instead of re-implementingbuild_fixture_library/dynamic_library_name/fixture_library_path.rust/lance/src/dataset/tests/dataset_index.rs#L2593-L2643: replace thedynamic_cache_fixturemodule with a call into the same shared helper crate.
As per coding guidelines, "disable coverage for test utilities with #[cfg_attr(coverage, coverage(off))]" applies to rust/**/*.rs, and duplicate code across changed files should be consolidated.
📍 Affects 3 files
rust/lance-cache-abi/tests/xabi_dynamic_backend.rs#L61-L102(this comment)rust/lance-core/src/cache/dynamic.rs#L404-L445rust/lance/src/dataset/tests/dataset_index.rs#L2593-L2643
🤖 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-cache-abi/tests/xabi_dynamic_backend.rs` around lines 61 - 102,
Extract fixture_library_path, build_fixture_library, and dynamic_library_name
from rust/lance-cache-abi/tests/xabi_dynamic_backend.rs lines 61-102 into a
shared dev-dependencies-only test-support crate under rust/lance-cache-abi,
adding #[cfg_attr(coverage, coverage(off))] to each function. Update
rust/lance-core/src/cache/dynamic.rs lines 404-445 to use the shared helper
instead of its duplicated implementation, and replace the dynamic_cache_fixture
module in rust/lance/src/dataset/tests/dataset_index.rs lines 2593-2643 with the
same helper; preserve the existing fixture build and platform-specific library
path behavior at all sites.
Source: Coding guidelines
| async fn invalidate_prefix(&self, prefix: &str) { | ||
| self.fallback.invalidate_prefix(prefix).await; | ||
| // The dynamic ABI intentionally omits per-key removal while the key | ||
| // model settles. Clearing preserves correctness for invalidation. | ||
| if let Err(error) = self.backend.clear().await { | ||
| log::warn!("dynamic cache backend clear failed during prefix invalidation: {error}"); | ||
| } | ||
| self.serialized_entries.store(0, Ordering::Relaxed); | ||
| self.serialized_size_bytes.store(0, Ordering::Relaxed); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
invalidate_prefix clears the whole dynamic backend, not just the prefix.
Any call to invalidate_prefix (e.g. after a single dataset write/compaction) wipes self.backend entirely via clear(), discarding cached entries for every other key/dataset sharing this adapter instance — not just entries under prefix. The comment documents this as an accepted interim tradeoff while the ABI's key model settles, but if a single DynamicCacheBackendAdapter is shared across multiple datasets/sessions (as the pattern in dataset_index.rs's Session::with_index_cache_backend usage suggests is plausible), routine writes to one dataset would evict prewarmed index caches for unrelated datasets, undermining the very "no IO after prewarm" guarantee this PR sets out to demonstrate.
🤖 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-core/src/cache/dynamic.rs` around lines 198 - 207, The
invalidate_prefix method currently clears the entire dynamic backend, evicting
entries unrelated to the requested prefix. Update
DynamicCacheBackendAdapter::invalidate_prefix to invalidate only entries
belonging to prefix, preserving cached entries for other datasets or keys;
remove the unconditional backend.clear path and keep the counters consistent
with the entries actually removed.
| pub use dynamic::DynamicCacheBackendAdapter; | ||
| pub use entry_io::{CacheEntryReader, CacheEntryWriter}; | ||
| pub use lance_cache_abi as dynamic_cache_abi; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Scope the lance_cache_abi re-export instead of exposing the whole crate.
pub use lance_cache_abi as dynamic_cache_abi; makes every public item of lance-cache-abi part of lance-core's public API, coupling lance-core's SemVer surface to a still-prototype ABI crate. Only CacheKey128/DynamicCacheBackendHandle (used by dynamic.rs) appear needed downstream.
As per coding guidelines, "Prefer pub(crate) over pub for crate-internal items, and use pub use re-exports for the actual public API surface."
♻️ Narrower re-export
-pub use lance_cache_abi as dynamic_cache_abi;
+pub use lance_cache_abi::{CacheKey128, DynamicCacheBackendHandle};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub use dynamic::DynamicCacheBackendAdapter; | |
| pub use entry_io::{CacheEntryReader, CacheEntryWriter}; | |
| pub use lance_cache_abi as dynamic_cache_abi; | |
| pub use dynamic::DynamicCacheBackendAdapter; | |
| pub use entry_io::{CacheEntryReader, CacheEntryWriter}; | |
| pub use lance_cache_abi::{CacheKey128, DynamicCacheBackendHandle}; |
🤖 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-core/src/cache/mod.rs` around lines 59 - 61, Replace the broad
`lance_cache_abi` re-export with a scoped `dynamic_cache_abi` module or
equivalent re-export exposing only `CacheKey128` and
`DynamicCacheBackendHandle`, which are required by `dynamic.rs`; keep those
symbols accessible where needed without exposing the ABI crate’s remaining
public items through `lance-core`.
Source: Coding guidelines
e6587c8 to
9b0d254
Compare
d5c7aef to
2fd1655
Compare
|
@Xuanwo @westonpace @wjones127, please take a look. Thanks. |
2fd1655 to
19ab5ff
Compare
19ab5ff to
cb9a76a
Compare
Summary
Prototype a xabi-backed dynamic cache backend ABI instead of publishing the current hand-written C vtable as v1.
This keeps the Lance cache backend contract as a Rust async trait and lets xabi generate the C-compatible vtable,
Future/Waker polling, typed error transport, panic guards, and module lifetime plumbing.
Key choices:
0, not v1CacheKey128invalidate_prefixfrom the low-level contractVerification
cargo fmt --package lance-cache-abi --package lance-cache-xabi-fixtureCARGO_NET_OFFLINE=true cargo test -p lance-cache-abiCARGO_NET_OFFLINE=true cargo check -p lance-coreCARGO_NET_OFFLINE=true cargo clippy -p lance-cache-abi --tests -- -D warnings