feat: expose shared session APIs - #63
Conversation
There was a problem hiding this comment.
The explicit session handle matches the upstream Lance ownership model, preserves the existing open APIs, and correctly demonstrates metadata-cache reuse and lifetime independence across Rust, C, and C++.
A non-blocking risk remains in the pinned upstream implementation: cache namespaces include the dataset URI but not object-store options. If one session opens the same URI against different physical endpoints, separate sessions are the safe mitigation until store identity is incorporated into the upstream cache key.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The session API is otherwise implemented cleanly, but the pinned Lance revision does not isolate dataset caches by effective object-store binding. Exposing one shared session together with per-open storage options therefore permits cache reuse across distinct accounts or endpoints that use the same URI.
Before accepting this API, either reject same-session/same-URI reuse when the storage binding differs, or depend on an upstream implementation that gives every store-bound dataset cache a store-aware namespace. Separate sessions are the safe workaround until that contract exists.
| builder = builder.with_version(version); | ||
| } | ||
| if let Some(session) = session { | ||
| builder = builder.with_session(session.inner.clone()); |
There was a problem hiding this comment.
This passes the same session into a builder that has just accepted per-open storage options, but the pinned Lance caches do not incorporate those options or effective store identity. In e934cc2, the metadata namespace and index namespace are both just the raw URI, and datasets construct both from that URI here. A second open of the same URI through a different account or endpoint can therefore hit entries populated by the first binding.
This is the same limitation previously noted as a risk, but upstream #7721 supplies the concrete same-URI/cross-account failure path: one dataset observed the other account's index UUID or foreign metadata. Its store-aware namespace change closed unmerged, and the pinned source still retains the URI-only keys.
Please establish isolation before exposing this combination: reject same-session/same-URI reuse when the effective storage binding differs, or move to an upstream revision that namespaces every store-bound cache by binding. Using separate sessions is safe for callers, but leaving this as documentation would still make the public API permit cross-binding cache reuse.
jja725
left a comment
There was a problem hiding this comment.
LGTM, could you fix the conflict
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The merge-only revision leaves the session patch, Lance pin, and storage-binding isolation finding unchanged. A shared session still permits URI-keyed cache entries to be reused across different per-open storage bindings, so the cross-account correctness and isolation consequence remains.
Acceptance still requires either rejecting conflicting bindings for the same session and URI, or taking a comprehensive upstream change that namespaces every store-bound cache by effective binding. Partial upstream fixes do not satisfy that contract.
Summary
LanceSessionwith configurable index and metadata cache limitslance_dataset_open_with_sessionwhile preserving the existing open API