feat(cache): support register and switch pluggable cache backend#7683
Open
yanghua wants to merge 12 commits into
Open
feat(cache): support register and switch pluggable cache backend#7683yanghua wants to merge 12 commits into
yanghua wants to merge 12 commits into
Conversation
Add a process-wide registry so cache backends can be discovered by a stable string identifier (`kind`) instead of being wired into every call site. Third-party backends integrate by defining a `BackendBuildFn` and calling `register_backend(kind, build)` once at application startup; `build_from_config` then locates the constructor and hands it a `BackendConfig`. `BackendConfig` is intentionally FFI-friendly (`kind: String`, `options: HashMap<String, String>`) so the same config shape can be represented in Python (`dict[str, str]`), Java (`Map<String, String>`), and — if a future dynamic loader is added — across a stable ABI without changing the user-visible configuration model. Duplicate registration under the same `kind` returns an error rather than silently overriding an existing constructor. This turns "two crates claim the same backend name" from a mystery cache downgrade into a startup-time error the operator can fix. No backend is registered by default; a follow-up commit will auto-register `MokaCacheBackend` under `"moka"`.
Add `parse_backend_uri` / `build_from_uri` so bindings and config files
can select a cache backend with a compact single-string form
(`moka://?capacity=1073741824`) instead of a typed builder. Bindings
that already exchange strings (Python, Java, YAML config) benefit the
most.
The grammar is a predictable subset of RFC 3986:
uri ::= scheme ":" hier ( "?" query )?
scheme ::= ALPHA ( ALPHA | DIGIT | "+" | "-" | "." )*
hier ::= "//" authority path? | path
Mapping to `BackendConfig`:
* `scheme` (lower-cased) becomes `kind`, matching the registry key.
* The joined authority + path (with a leading "//" and any leading
"/" trimmed) is stored under the option `path` when non-empty. If
the query also sets `path`, the URI is rejected.
* Each `key=value` pair from the query is percent-decoded into
`options`. Duplicate keys and pairs without `=` are rejected.
`build_from_uri` composes `parse_backend_uri` with `build_from_config`.
Errors surface the offending URI so misconfiguration is easy to trace.
Ten unit tests cover authority-only URIs, path-and-query URIs,
host-style URIs (`redis://localhost:6379/0`), case-insensitive
schemes, percent-decoding, trailing "&", missing scheme, invalid
scheme, duplicate query keys, and pairs without "=".
Make the Moka backend discoverable through the registry so a bare
Lance installation can build a cache backend from a `BackendConfig` or
URI (`moka://?capacity=...`) without the caller registering it
first. This lets bindings expose a single "cache backend" knob whose
default value ("moka") works everywhere.
Changes:
* `MokaCacheBackend` now has a `MOKA_BACKEND_KIND` identifier and a
`build_moka` `BackendBuildFn`. The build fn recognizes a single
option (`capacity` in bytes), rejects unknown options so typos
surface at construction time, and defaults to a 0-capacity
(disabled) cache when `capacity` is missing.
* `build_from_config` now calls a private `ensure_builtin_backends`
helper before looking up the requested kind. The helper checks
whether Moka is already registered and inserts it if not; the
check is against the current registry contents (rather than a
process-once flag) so `#[cfg(test)]` helpers that snapshot and
restore the registry still see the built-in backend after they
take ownership.
* Third-party backends are unchanged — they still opt in with their
own `register()` call, and the registry rejects duplicate `kind`
registrations.
Four unit tests cover the config path, the URI path, unknown-option
rejection, and capacity-parse errors.
Add `Session::with_cache_backends(index_backend, metadata_backend, ...)`
so callers who have already resolved backend selection through
`lance_core::cache::build_from_config` / `build_from_uri` can inject a
backend for either or both caches in one call. Each argument is
optional; `None` falls back to the same size-driven default that
`Session::new` would have produced.
This closes the asymmetry between the existing `with_index_cache_backend`
(index only) and the metadata cache (previously never receiving a
custom backend at all), and gives language bindings a single Rust
entry point to wire up `index_cache_backend` / `metadata_cache_backend`
parameters.
A unit test verifies the "inject one, default the other" flow using
`build_from_uri("moka://?capacity=...")` end-to-end.
Add `index_cache_backend` and `metadata_cache_backend` keyword arguments
to `lance.Session(...)`. Each accepts either a URI string
(`"moka://?capacity=..."`) that is resolved through
`lance_core::cache::build_from_uri`, or a dict of the form
`{"kind": "moka", "options": {"capacity": "..."}}` that is resolved
through `build_from_config`.
Setting both `*_cache_size_bytes` and `*_cache_backend` for the same
cache raises `ValueError` at binding boundary rather than silently
letting one override the other. Unknown Python types on the backend
kwargs also raise `ValueError` with a clear message pointing at the
offending argument.
Internally the constructor now routes through
`LanceSession::with_cache_backends`, which falls back to the size-based
default when the caller passes `None`, so the previous
`Session(index_cache_size_bytes=..., metadata_cache_size_bytes=...)`
call path is unchanged.
github-actions
Bot
added
A-python
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds cache-backend configuration and URI parsing in ChangesConfigurable cache backends for Lance Session
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
python/python/tests/test_session.py (1)
43-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParametrize the two positive backend-config tests.
test_cache_backend_uri_configandtest_cache_backend_dict_configdiffer only by theindex_cache_backendinput and share the same assertion, so they should be a single parametrized test.As per coding guidelines: "use
@pytest.mark.parametrizefor Python tests when cases differ only by inputs".♻️ Proposed parametrization
-def test_cache_backend_uri_config(): - session = lance.Session(index_cache_backend="moka://?capacity=1048576") - - assert session.index_cache_size_bytes() == 0 - - -def test_cache_backend_dict_config(): - session = lance.Session( - index_cache_backend={ - "kind": "moka", - "options": {"capacity": "1048576"}, - }, - ) - - assert session.index_cache_size_bytes() == 0 +@pytest.mark.parametrize( + "index_cache_backend", + [ + "moka://?capacity=1048576", + {"kind": "moka", "options": {"capacity": "1048576"}}, + ], + ids=["uri", "dict"], +) +def test_cache_backend_config(index_cache_backend): + session = lance.Session(index_cache_backend=index_cache_backend) + + assert session.index_cache_size_bytes() == 0🤖 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 `@python/python/tests/test_session.py` around lines 43 - 57, Combine test_cache_backend_uri_config and test_cache_backend_dict_config into one pytest parametrized test in test_session.py, since they only vary by the index_cache_backend input and share the same assertion. Use `@pytest.mark.parametrize` on the test function and pass both existing backend-config inputs as cases, keeping the assertion on Session.index_cache_size_bytes() unchanged.Source: Coding guidelines
rust/lance-core/src/cache/backend_uri.rs (1)
29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfusing wording in the path-conflict doc note.
"the URI-supplied path wins and the parser errors on the conflict" reads as contradictory — the actual behavior (per
parse_backend_uri, Lines 93-98) is that a conflictingpathquery key always errors, nothing "wins".🤖 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/backend_uri.rs` around lines 29 - 34, Update the doc note in backend_uri so it matches `parse_backend_uri` behavior: the `path` query key is not overridden by the URI path on conflict, it simply causes an error. Reword the comment near the authority/path handling to state that the joined URI path is stored only when no `path` query key exists, and that a conflicting `path` key results in a parser error rather than one value “winning.”rust/lance-core/src/cache/registry.rs (1)
35-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding runnable examples to key public APIs.
BackendConfig::new/with_option(Lines 37-48) andbuild_from_config(Lines 107-123) are public but lack usage examples;register_backend's doc (Lines 62-76) does include one.As per coding guidelines, "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods."
Also applies to: 107-123
🤖 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/registry.rs` around lines 35 - 49, Add runnable doc examples to the public APIs in BackendConfig and registry helpers: document BackendConfig::new and BackendConfig::with_option with a usage example, and add an example to build_from_config as well. Keep the examples consistent with the existing register_backend docs, and make sure the docs link to the relevant types and methods (BackendConfig, register_backend, build_from_config) so the public API guidance is satisfied.Source: Coding guidelines
rust/lance/src/session.rs (1)
160-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
with_index_cache_backendis now redundant; consider deprecating it.
with_cache_backends(160-193) supersedeswith_index_cache_backend(121-133) — both let a caller inject a custom index backend, withwith_cache_backendsadditionally covering the metadata tier. Leaving both as separate public constructors invites drift/confusion, and per coding guidelines, obsolete public constructors should follow a deprecation path rather than remain side-by-side indefinitely. Separately, both are ad-hoc constructor variants rather thanwith_-style builder methods chained ontoSession::new(...), which the guidelines call out to avoid.As per coding guidelines, "Delete obsolete internal (
pub(crate)or private) methods in the same PR that introduces their replacements; for public API methods, follow the deprecation path in root AGENTS.md instead" and "Usewith_-prefixed builder methods for optional config ... and do not create separate constructor variants."🤖 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/session.rs` around lines 160 - 193, `with_index_cache_backend` is now superseded by `Session::with_cache_backends`, so update the public API to follow the deprecation path instead of keeping both constructors active. Add deprecation guidance on `with_index_cache_backend` and route callers toward `with_cache_backends`, which already covers the index backend plus metadata backend. Keep the implementation behavior consistent in `Session` while avoiding a long-lived redundant constructor variant that can drift from `with_cache_backends`.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-core/src/cache/backend_uri.rs`:
- Around line 162-188: The `percent_decode` helper in `backend_uri.rs` is
applying form-style `+` to space decoding, which conflicts with the RFC 3986
subset described by the module docs. Update `percent_decode` to stop treating
`+` as a space so it only performs percent-escape decoding, and keep the
`BackendUri` parsing behavior consistent with the documented grammar. If you
prefer to preserve `+` handling, then explicitly document that exception in the
module’s RFC section so callers know literal `+` must be encoded as `%2B`.
In `@rust/lance-core/src/cache/registry.rs`:
- Around line 99-105: Move the inline imports in cache::registry to file scope:
relocate the super::moka::{MOKA_BACKEND_KIND, build_moka} and
super::moka::MOKA_BACKEND_KIND uses out of builtin_backend and the other
affected function block(s), then keep those functions referring to the
already-imported symbols. Update the top-level use declarations near the rest of
the module imports so builtin_backend and the related registry logic no longer
contain inline use statements.
- Line 88: The registry lock handling in `registry()` and the other affected
registry access points still uses a bare mutex `.unwrap()`, which can panic in
library code if the lock is poisoned. Replace the `registry().lock().unwrap()`
pattern at the identified registry methods with the same poisoned-lock recovery
used in the test helpers, i.e. recover the inner guard instead of panicking, and
apply the fix consistently across the referenced registry functions so all
production lock acquisitions behave safely.
In `@rust/lance/src/session.rs`:
- Around line 409-433: The test in Session::with_cache_backends only checks a
miss on an unrelated key, so it doesn’t prove the injected index backend from
build_from_uri was actually wired in. Update
test_with_cache_backends_uses_provided_and_default to exercise the
session.index_cache with a value inserted through the injected backend and then
read it back (or otherwise assert a behavior unique to the injected cache),
while keeping the metadata_cache fallback check as-is.
- Around line 160-170: The public API doc for Session::with_cache_backends is
missing a usage example, so add a rust,no_run example to the doc comment near
with_cache_backends that demonstrates constructing a cache backend and calling
Session::with_cache_backends, similar to the existing with_spill_store
documentation. Make sure the example references Session and
build_from_uri/build_from_config as appropriate and includes the relevant doc
links so the new API follows the project’s public-documentation guidelines.
---
Nitpick comments:
In `@python/python/tests/test_session.py`:
- Around line 43-57: Combine test_cache_backend_uri_config and
test_cache_backend_dict_config into one pytest parametrized test in
test_session.py, since they only vary by the index_cache_backend input and share
the same assertion. Use `@pytest.mark.parametrize` on the test function and pass
both existing backend-config inputs as cases, keeping the assertion on
Session.index_cache_size_bytes() unchanged.
In `@rust/lance-core/src/cache/backend_uri.rs`:
- Around line 29-34: Update the doc note in backend_uri so it matches
`parse_backend_uri` behavior: the `path` query key is not overridden by the URI
path on conflict, it simply causes an error. Reword the comment near the
authority/path handling to state that the joined URI path is stored only when no
`path` query key exists, and that a conflicting `path` key results in a parser
error rather than one value “winning.”
In `@rust/lance-core/src/cache/registry.rs`:
- Around line 35-49: Add runnable doc examples to the public APIs in
BackendConfig and registry helpers: document BackendConfig::new and
BackendConfig::with_option with a usage example, and add an example to
build_from_config as well. Keep the examples consistent with the existing
register_backend docs, and make sure the docs link to the relevant types and
methods (BackendConfig, register_backend, build_from_config) so the public API
guidance is satisfied.
In `@rust/lance/src/session.rs`:
- Around line 160-193: `with_index_cache_backend` is now superseded by
`Session::with_cache_backends`, so update the public API to follow the
deprecation path instead of keeping both constructors active. Add deprecation
guidance on `with_index_cache_backend` and route callers toward
`with_cache_backends`, which already covers the index backend plus metadata
backend. Keep the implementation behavior consistent in `Session` while avoiding
a long-lived redundant constructor variant that can drift from
`with_cache_backends`.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13eee651-c894-44b3-a7ca-be53c0ca8503
📒 Files selected for processing (8)
python/python/lance/lance/__init__.pyipython/python/tests/test_session.pypython/src/session.rsrust/lance-core/src/cache/backend_uri.rsrust/lance-core/src/cache/mod.rsrust/lance-core/src/cache/moka.rsrust/lance-core/src/cache/registry.rsrust/lance/src/session.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the phase-1 framework proposed in
discussion #7575:
a registry so cache backends can be selected by a stable string identifier
(
kind) and configured through either a structuredBackendConfigor acompact URI.
MokaCacheBackendis auto-registered under"moka"so nouser-visible behavior changes for the default configuration.
This PR intentionally ships only the mechanism. It does not add a new
concrete backend implementation. Third-party crates (compiled together with
Lance) integrate by defining their own
BackendBuildFnand callingregister_backend(kind, build)once at startup. A future PR can add adynamic-library loader on top of the same registry without changing the
user-facing configuration model.
What lands
BackendConfig+ registryBackendConfig { kind, options: HashMap<String, String> }and aprocess-wide registry (
register_backend,build_from_config). Optionsare FFI-friendly string pairs so the same shape carries across Python
dict[str, str], JavaMap<String, String>, or a future stable ABI.Duplicate
kindregistration returns an error rather than silentlyoverriding an existing constructor.
URI form
parse_backend_uri/build_from_uriaccept a compact single-stringform (
moka://?capacity=1073741824). Grammar is a small, predictablesubset of RFC 3986:
The scheme becomes
kind; the joined authority+path (if any) is storedunder the option
path; query pairs are percent-decoded. Duplicate querykeys, pairs without
=, and invalid schemes are rejected.Auto-register
MokaCacheBackendas"moka"A private
ensure_builtin_backends()runs on the first call tobuild_from_config/build_from_uri, so a bare Lance install canbuild a Moka backend from a URI/config without the caller registering
it first. The build fn recognises a single option (
capacityin bytes)and rejects unknown options so typos surface at construction time.
Symmetric
Session::with_cache_backendsNew constructor takes
Option<Arc<dyn CacheBackend>>for both the indexand metadata caches. Whichever tier the caller passes
Nonefor fallsback to the same size-based default that
Session::newwould have used.The existing
Session::new/Session::with_index_cache_backendentrypoints are unchanged.
Python
Session(index_cache_backend=..., metadata_cache_backend=...)Each accepts either a URI
str("moka://?capacity=...") resolvedthrough
build_from_uri, or adictof the form{"kind": "moka", "options": {"capacity": "..."}}resolved throughbuild_from_config. Passing both*_cache_size_bytesand*_cache_backendfor the same cache raisesValueErrorat the bindingboundary rather than silently letting one override the other
(proposal §7).
What this PR intentionally does not do
external crates and register themselves with
register_backend(...)..so/ C-ABI loader. The design keeps room for it later(string-map config, conflict-detecting registry, byte-oriented
values via
CacheCodec), but that's a follow-up.CacheBackendtrait itself.Session::new(index_cache_size, metadata_cache_size, ...))is byte-for-byte the same as before.
Usage sketch
Rust:
A hypothetical third-party crate:
Python:
Test plan
cargo check -p lance-core --testscargo check -p lance --testscargo check --manifest-path python/Cargo.tomlcargo test -p lance-core --lib cache::— 39 passed (14 new: 4 registry+ 10 URI parser + 4 moka via config/URI)
cargo test -p lance --lib session::tests— 10 passed (1 new:test_with_cache_backends_uses_provided_and_default)Session(index_cache_backend="moka://...")roundtrip)— reviewers who prefer a wheel-based smoke test, please shout and I'll add
one.
Discussion link
Design context and open questions:
Discussion #7575 — Proposal: Pluggable cache backends
Summary by CodeRabbit
capacityoption).