feat: add dynamic cache ABI crate#7828
Conversation
📝 WalkthroughWalkthroughChangesDynamic cache ABI
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
🟡 Other comments (1)
rust/lance-cache-abi/src/lib.rs-29-43 (1)
29-43: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
from_slicedoesn't null-normalizeptrwhenlen == 0, violating the field doc.The field doc on line 30 states "or null when
len == 0", butfrom_slice(&[])produces a non-null dangling pointer (Rust's empty-sliceas_ptr()is non-null). A C backend that checksptr != NULLinstead oflen > 0could dereference it. The same invariant gap exists onDynamicCacheOwnedBytes::from_vec(lines 86-97) whenlen == 0butcapacity > 0.🛡️ Proposed fix for both constructors
pub fn from_slice(bytes: &[u8]) -> Self { Self { - ptr: bytes.as_ptr(), + ptr: if bytes.is_empty() { std::ptr::null() } else { bytes.as_ptr() }, len: bytes.len(), } }And for
DynamicCacheOwnedBytes::from_vec:pub fn from_vec(mut bytes: Vec<u8>) -> Self { - if bytes.capacity() == 0 { + if bytes.is_empty() { return Self::empty(); }🤖 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/lib.rs` around lines 29 - 43, Update DynamicCacheByteSlice::from_slice and DynamicCacheOwnedBytes::from_vec to set ptr to null whenever the resulting len is zero, including empty vectors with nonzero capacity; preserve the existing data pointer for non-empty values and keep ownership metadata unchanged.
🧹 Nitpick comments (2)
rust/lance-cache-abi/src/lib.rs (2)
252-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
DynamicCacheByteSliceround-trip and empty-slice handling.The only test covers
DynamicCacheOwnedBytes.DynamicCacheByteSlice::from_slice/as_sliceand the empty-slice null-pointer edge case are untested. As per coding guidelines, every feature must have corresponding tests.🧪 Suggested additional tests
#[test] fn owned_bytes_round_trip_in_allocating_library() { let bytes = vec![1, 2, 3]; let owned = DynamicCacheOwnedBytes::from_vec(bytes); // SAFETY: This test reconstructs the vector in the same crate that // allocated it, which mirrors a plugin's `free_bytes` implementation. let bytes = unsafe { owned.into_vec() }; assert_eq!(bytes, vec![1, 2, 3]); } + + #[test] + fn byte_slice_round_trip() { + let data = b"hello"; + let slice = DynamicCacheByteSlice::from_slice(data); + // SAFETY: `data` outlives the call. + let borrowed = unsafe { slice.as_slice() }; + assert_eq!(borrowed, b"hello"); + } + + #[test] + fn byte_slice_empty_is_safe() { + let slice = DynamicCacheByteSlice::from_slice(&[]); + assert_eq!(slice.len, 0); + // SAFETY: len == 0 returns an empty slice without dereferencing ptr. + let borrowed = unsafe { slice.as_slice() }; + assert!(borrowed.is_empty()); + }🤖 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/lib.rs` around lines 252 - 265, Add tests in the existing tests module covering DynamicCacheByteSlice::from_slice followed by as_slice, asserting the original bytes are preserved, and separately covering an empty slice to verify its null-pointer edge-case behavior. Keep the existing DynamicCacheOwnedBytes test unchanged.Source: Coding guidelines
14-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
&CStrinstead of&[u8]for the init symbol.
&[u8]with an embedded null terminator is error-prone for FFI symbol lookup. A&CStrprovides type safety and direct.as_ptr()access fordlsym/GetProcAddress. C string literals are const-constructible since Rust 1.77, which the project exceeds.♻️ Proposed refactor
-use std::ffi::c_void; +use std::ffi::{c_void, CStr}; -/// Exported symbol every dynamic cache backend must provide. -pub const DYNAMIC_CACHE_BACKEND_INIT_SYMBOL: &[u8] = b"lance_cache_backend_init\0"; +/// Exported symbol every dynamic cache backend must provide. +pub const DYNAMIC_CACHE_BACKEND_INIT_SYMBOL: &CStr = c"lance_cache_backend_init";🤖 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/lib.rs` around lines 14 - 15, Change DYNAMIC_CACHE_BACKEND_INIT_SYMBOL from a byte slice to a const-constructible &CStr using the existing null-terminated symbol value. Preserve the exported symbol name and ensure callers use its direct pointer access for dynamic lookup.
🤖 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.
Other comments:
In `@rust/lance-cache-abi/src/lib.rs`:
- Around line 29-43: Update DynamicCacheByteSlice::from_slice and
DynamicCacheOwnedBytes::from_vec to set ptr to null whenever the resulting len
is zero, including empty vectors with nonzero capacity; preserve the existing
data pointer for non-empty values and keep ownership metadata unchanged.
---
Nitpick comments:
In `@rust/lance-cache-abi/src/lib.rs`:
- Around line 252-265: Add tests in the existing tests module covering
DynamicCacheByteSlice::from_slice followed by as_slice, asserting the original
bytes are preserved, and separately covering an empty slice to verify its
null-pointer edge-case behavior. Keep the existing DynamicCacheOwnedBytes test
unchanged.
- Around line 14-15: Change DYNAMIC_CACHE_BACKEND_INIT_SYMBOL from a byte slice
to a const-constructible &CStr using the existing null-terminated symbol value.
Preserve the exported symbol name and ensure callers use its direct pointer
access for dynamic lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 9dcd6ea4-9d7a-4d74-be47-bb2707b72275
⛔ 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 (5)
Cargo.tomlrust/lance-cache-abi/Cargo.tomlrust/lance-cache-abi/src/lib.rsrust/lance-core/Cargo.tomlrust/lance-core/src/cache/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/lib.rs`:
- Around line 166-169: Define one canonical representation for empty owned
buffers in the buffer contract: require len == 0 to use a null ptr and capacity
== 0, and update the related documentation around the owned-buffer definition
and free_bytes behavior to match. Ensure the empty-buffer no-op semantics cannot
permit an allocated zero-length buffer to leak.
- Around line 67-120: Add compiling Rust doc examples across the public ABI APIs
in the affected sections, covering owned-byte lifecycle and unsafe ownership
rules, DynamicCacheStatus conversion, vtable construction, and the
initialization handshake. Attach each example to the relevant public structs,
enums, constants, and methods using intra-doc links, and ensure every snippet
matches the current function signatures and compiles as documentation tests.
- Around line 381-385: Make vtable initialization reject any host buffer smaller
than DYNAMIC_CACHE_BACKEND_VTABLE_SIZE before calling out_vtable.write, keeping
new() and the documented full-size protocol consistent. Preserve full-size
initialization behavior and add tests covering required-size rejection, exact
full-size success, and a canary buffer proving no out-of-bounds writes occur.
- Around line 423-449: Update the current-layout constructor
DynamicCacheVTable::new to stop accepting an abi_version argument and always
assign DYNAMIC_CACHE_ABI_VERSION internally. Keep the constructor’s other
callback parameters and current-layout metadata unchanged; add version-specific
constructors only if another layout is actually supported.
In `@rust/lance-core/src/cache/mod.rs`:
- Line 58: Update the cache module’s re-exports so the existing direct ABI
symbols, including DynamicCacheBackendVTable, remain publicly available. Add the
dynamic_cache_abi module alias alongside those direct re-exports, and apply the
project’s standard #[deprecated] compatibility treatment to the old paths only
after providing the replacement.
🪄 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: 0132d7ef-0fff-48d5-b0df-d9a3933da58c
📒 Files selected for processing (2)
rust/lance-cache-abi/src/lib.rsrust/lance-core/src/cache/mod.rs
| /// Dynamic cache operation status. | ||
| /// | ||
| /// ABI function signatures use raw `u32` return values so hosts can handle | ||
| /// unknown future status codes without constructing an invalid Rust enum. This | ||
| /// enum is a Rust-side helper for known v1 status values. | ||
| #[repr(u32)] | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| pub enum DynamicCacheStatus { | ||
| /// Operation completed successfully. | ||
| Ok = 0, | ||
| /// Lookup found a byte payload. | ||
| Hit = 1, | ||
| /// Lookup did not find a byte payload. | ||
| Miss = 2, | ||
| /// Operation failed inside the dynamic backend. | ||
| Error = 3, | ||
| } | ||
|
|
||
| impl DynamicCacheStatus { | ||
| /// Return the raw ABI status code. | ||
| pub const fn as_u32(self) -> u32 { | ||
| self as u32 | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<u32> for DynamicCacheStatus { | ||
| type Error = UnknownDynamicCacheStatus; | ||
|
|
||
| fn try_from(value: u32) -> Result<Self, UnknownDynamicCacheStatus> { | ||
| match value { | ||
| DYNAMIC_CACHE_STATUS_OK => Ok(Self::Ok), | ||
| DYNAMIC_CACHE_STATUS_HIT => Ok(Self::Hit), | ||
| DYNAMIC_CACHE_STATUS_MISS => Ok(Self::Miss), | ||
| DYNAMIC_CACHE_STATUS_ERROR => Ok(Self::Error), | ||
| _ => Err(UnknownDynamicCacheStatus(value)), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Raw status code that is not known to this ABI crate. | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| pub struct UnknownDynamicCacheStatus(pub u32); | ||
|
|
||
| /// Operation completed successfully. | ||
| pub const DYNAMIC_CACHE_STATUS_OK: u32 = DynamicCacheStatus::Ok as u32; | ||
| /// Lookup found a byte payload. | ||
| pub const DYNAMIC_CACHE_STATUS_HIT: u32 = DynamicCacheStatus::Hit as u32; | ||
| /// Lookup did not find a byte payload. | ||
| pub const DYNAMIC_CACHE_STATUS_MISS: u32 = DynamicCacheStatus::Miss as u32; | ||
| /// Operation failed inside the dynamic backend. | ||
| pub const DYNAMIC_CACHE_STATUS_ERROR: u32 = DynamicCacheStatus::Error as u32; | ||
|
|
||
| /// Number of pointer-sized slots reserved for future optional functions. | ||
| pub const DYNAMIC_CACHE_BACKEND_VTABLE_RESERVED_SLOTS: usize = 8; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add compiling examples for the public ABI surface.
Document the owned-byte lifecycle, status conversion, vtable construction, and initialization handshake with examples linked to the relevant types. This is especially important for the unsafe ownership contracts.
As per coding guidelines, “Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.”
Also applies to: 122-220, 274-359, 368-477
🤖 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/lib.rs` around lines 67 - 120, Add compiling Rust
doc examples across the public ABI APIs in the affected sections, covering
owned-byte lifecycle and unsafe ownership rules, DynamicCacheStatus conversion,
vtable construction, and the initialization handshake. Attach each example to
the relevant public structs, enums, constants, and methods using intra-doc
links, and ensure every snippet matches the current function signatures and
compiles as documentation tests.
Source: Coding guidelines
| /// | ||
| /// A non-empty buffer must have a non-null `ptr`, `capacity >= len`, and an | ||
| /// allocation that can be released by the backend's [`DynamicCacheFreeBytesFn`]. | ||
| /// Empty buffers have no readable bytes; hosts must not read from `ptr`. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Define canonical empty owned-buffer semantics.
The general contract permits len == 0 with nonzero capacity, but free_bytes says empty buffers should be no-ops. A backend returning an allocated empty buffer could therefore leak it. Either require ptr == null && capacity == 0 whenever len == 0, or require free_bytes to release nonzero-capacity empty allocations.
Also applies to: 360-366
🤖 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/lib.rs` around lines 166 - 169, Define one canonical
representation for empty owned buffers in the buffer contract: require len == 0
to use a null ptr and capacity == 0, and update the related documentation around
the owned-buffer definition and free_bytes behavior to match. Ensure the
empty-buffer no-op semantics cannot permit an allocated zero-length buffer to
leak.
| /// Size in bytes of the vtable filled by the backend. | ||
| /// | ||
| /// Backends must set this to [`DYNAMIC_CACHE_BACKEND_VTABLE_SIZE`]. Hosts | ||
| /// must validate this before reading fields that may not exist in older | ||
| /// vtables. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make bounded vtable initialization unambiguous and safe.
Initialization permits host_vtable_size < DYNAMIC_CACHE_BACKEND_VTABLE_SIZE, while the field documentation and new() produce a full-sized vtable. Writing that value with out_vtable.write(...) would overrun a smaller host buffer.
Define one safe protocol: reject undersized buffers, or provide prefix-writing logic that writes at most host_vtable_size and reports the exact bytes written. Add required-size, full-size, and canary-buffer tests.
As per coding guidelines, “Every bugfix and feature must have corresponding tests.”
Also applies to: 423-449, 465-477
🤖 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/lib.rs` around lines 381 - 385, Make vtable
initialization reject any host buffer smaller than
DYNAMIC_CACHE_BACKEND_VTABLE_SIZE before calling out_vtable.write, keeping new()
and the documented full-size protocol consistent. Preserve full-size
initialization behavior and add tests covering required-size rejection, exact
full-size success, and a canary buffer proving no out-of-bounds writes occur.
Source: Coding guidelines
| /// Create a vtable with current layout metadata and empty reserved slots. | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub const fn new( | ||
| abi_version: u32, | ||
| create: DynamicCacheCreateFn, | ||
| destroy: DynamicCacheDestroyFn, | ||
| get: DynamicCacheGetFn, | ||
| insert: DynamicCacheInsertFn, | ||
| invalidate_prefix: DynamicCacheInvalidatePrefixFn, | ||
| clear: DynamicCacheClearFn, | ||
| measure: DynamicCacheMeasureFn, | ||
| free_bytes: DynamicCacheFreeBytesFn, | ||
| ) -> Self { | ||
| Self { | ||
| struct_size: DYNAMIC_CACHE_BACKEND_VTABLE_SIZE, | ||
| abi_version, | ||
| create, | ||
| destroy, | ||
| get, | ||
| insert, | ||
| invalidate_prefix, | ||
| clear, | ||
| measure, | ||
| free_bytes, | ||
| reserved: [None; DYNAMIC_CACHE_BACKEND_VTABLE_RESERVED_SLOTS], | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not accept arbitrary ABI versions for the current-layout constructor.
new() always creates the current vtable layout, but callers can label it with any u32. Set abi_version to DYNAMIC_CACHE_ABI_VERSION internally; introduce explicit version-specific constructors only when another layout exists.
Proposed fix
pub const fn new(
- abi_version: u32,
create: DynamicCacheCreateFn,
destroy: DynamicCacheDestroyFn,
get: DynamicCacheGetFn,
insert: DynamicCacheInsertFn,
invalidate_prefix: DynamicCacheInvalidatePrefixFn,
clear: DynamicCacheClearFn,
measure: DynamicCacheMeasureFn,
free_bytes: DynamicCacheFreeBytesFn,
) -> Self {
Self {
struct_size: DYNAMIC_CACHE_BACKEND_VTABLE_SIZE,
- abi_version,
+ abi_version: DYNAMIC_CACHE_ABI_VERSION,As per coding guidelines, “Validate inputs at API boundaries and reject invalid values with descriptive errors; never silently clamp or adjust them.”
📝 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.
| /// Create a vtable with current layout metadata and empty reserved slots. | |
| #[allow(clippy::too_many_arguments)] | |
| pub const fn new( | |
| abi_version: u32, | |
| create: DynamicCacheCreateFn, | |
| destroy: DynamicCacheDestroyFn, | |
| get: DynamicCacheGetFn, | |
| insert: DynamicCacheInsertFn, | |
| invalidate_prefix: DynamicCacheInvalidatePrefixFn, | |
| clear: DynamicCacheClearFn, | |
| measure: DynamicCacheMeasureFn, | |
| free_bytes: DynamicCacheFreeBytesFn, | |
| ) -> Self { | |
| Self { | |
| struct_size: DYNAMIC_CACHE_BACKEND_VTABLE_SIZE, | |
| abi_version, | |
| create, | |
| destroy, | |
| get, | |
| insert, | |
| invalidate_prefix, | |
| clear, | |
| measure, | |
| free_bytes, | |
| reserved: [None; DYNAMIC_CACHE_BACKEND_VTABLE_RESERVED_SLOTS], | |
| } | |
| } | |
| /// Create a vtable with current layout metadata and empty reserved slots. | |
| #[allow(clippy::too_many_arguments)] | |
| pub const fn new( | |
| create: DynamicCacheCreateFn, | |
| destroy: DynamicCacheDestroyFn, | |
| get: DynamicCacheGetFn, | |
| insert: DynamicCacheInsertFn, | |
| invalidate_prefix: DynamicCacheInvalidatePrefixFn, | |
| clear: DynamicCacheClearFn, | |
| measure: DynamicCacheMeasureFn, | |
| free_bytes: DynamicCacheFreeBytesFn, | |
| ) -> Self { | |
| Self { | |
| struct_size: DYNAMIC_CACHE_BACKEND_VTABLE_SIZE, | |
| abi_version: DYNAMIC_CACHE_ABI_VERSION, | |
| create, | |
| destroy, | |
| get, | |
| insert, | |
| invalidate_prefix, | |
| clear, | |
| measure, | |
| free_bytes, | |
| reserved: [None; DYNAMIC_CACHE_BACKEND_VTABLE_RESERVED_SLOTS], | |
| } | |
| } |
🤖 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/lib.rs` around lines 423 - 449, Update the
current-layout constructor DynamicCacheVTable::new to stop accepting an
abi_version argument and always assign DYNAMIC_CACHE_ABI_VERSION internally.
Keep the constructor’s other callback parameters and current-layout metadata
unchanged; add version-specific constructors only if another layout is actually
supported.
Source: Coding guidelines
| CacheCodec, CacheCodecImpl, CacheDecode, CacheMissReason, MAGIC, has_cache_envelope, | ||
| }; | ||
| pub use entry_io::{CacheEntryReader, CacheEntryWriter}; | ||
| pub use lance_cache_abi as dynamic_cache_abi; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the existing direct ABI re-exports.
Replacing the previous symbol re-exports breaks imports such as lance_core::cache::DynamicCacheBackendVTable. Add the module alias alongside the existing paths, then deprecate those paths through the normal compatibility process.
As per coding guidelines, “Do not break public API signatures; deprecate old APIs with #[deprecated] and add a replacement.”
🤖 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` at line 58, Update the cache module’s
re-exports so the existing direct ABI symbols, including
DynamicCacheBackendVTable, remain publicly available. Add the dynamic_cache_abi
module alias alongside those direct re-exports, and apply the project’s standard
#[deprecated] compatibility treatment to the old paths only after providing the
replacement.
Source: Coding guidelines
1 participant
Summary
This PR adds a standalone
lance-cache-abicrate for the dynamic cache backend C ABI.The new crate contains only the ABI-facing definitions:
lance-coredepends on this crate and re-exports the ABI types fromlance_core::cache, so future dynamic cache backend implementations can depend onthe ABI crate directly without depending on
lance-core.This PR intentionally does not add dynamic loading logic or any concrete cache backend implementation.
Testing
Not run locally after the latest rebase because the local environment stalled while syncing the Rust
1.97.0toolchain components.