fix(catalog): describe-first table resolution for REST namespaces - #229
fix(catalog): describe-first table resolution for REST namespaces#229hellower wants to merge 3 commits into
Conversation
The lazy-discovery guard added by lance-format#220 to LanceRestNamespaceDefaultGenerator::CreateDefaultEntry gates every point lookup on membership in GetDefaultEntries(), which has three defects: 1. The membership list runs with ATTACH-time captured credentials, before ResolveLanceNamespaceAuth re-resolves DuckDB secrets: after a secret rotation, lookups of not-yet-materialized entries fail with stale credentials even though the describe/open path below would succeed. 2. With a non-empty namespace id the guard compares the raw entry name against prefix-stripped listed names, so an already-qualified reference (ns$tbl) is rejected before the candidate logic (which deliberately describes prefixed names as-is) can run. 3. List permission becomes a prerequisite for point lookups: credentials that allow describe/open but deny list_tables, or tables intentionally omitted from listings, are misreported as not found. Replace the gate with describe-first resolution: resolve auth first, describe the candidates with three-state outcomes (Found / NotFound / Error), where NotFound is a new typed signal (ErrorCode::NamespaceTableNotFound) classified in Rust from spec-conformant not-found errors only. Precedence over the canonical (namespace-qualified, always last) candidate: - any candidate found-but-unconvertible -> empty entry (table exists); - canonical typed NotFound -> nullptr (soft fall-through to the system catalog, the guard's original purpose, suppressing alias arity noise); - canonical infra Error -> membership fallback with freshly resolved credentials: unlisted -> nullptr, listed -> empty entry, list fails -> surface the describe error as an IOException (never masquerade a possibly-existing table as not-found). The thread-local FFI error is consumed on every describe path. Error code 55 is intentionally left unused so this change and the in-flight change that claims it can merge in either order. Also registers rust/ffi/namespace.rs in RUST_FFI_DEPENDS so incremental builds pick up edits to the file. Refs lance-format#220.
…r test Narrow the per-case unsafe region to the two operations that need it (the FFI describe call and the error-message reclamation) and document their lifetime and ownership invariants, per docs/rust_guidelines.md.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea923158c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…FI errors Address two review findings on the describe-first resolution: 1. Cross-namespace alias resolution. The bare one-segment candidate was described before the namespace-qualified id and any success returned immediately. On a hierarchical REST server a one-segment id addresses the ROOT namespace, so a root table could satisfy -- or, when both exist, shadow -- a lookup made through an attached child namespace. Reorder candidates canonical-FIRST and double-guard the alias: once the canonical id proves existence (describe Found or open succeeded, convertibility aside) the alias is never consulted, and an alias success only counts after a memoized membership listing confirms the name belongs to the attached namespace (checked lazily after the success, so system-name probes never pay a list round trip). The canonical-Error fallback tail reuses the same memoized probe. Trade-off: on server dialects that store unprefixed ids, alias resolution now requires list permission; the canonical path still needs none. 2. Thread-local FFI error leak. The slow fallback's failed-open paths (nullptr return and the unresolvable-candidate exception arm) left the FFI error in thread-local storage on the soft not-found return, where an unrelated LanceFormatErrorSuffix() on the same thread would later report it. Consume the error on both paths. The empty-entry tail keeps the bare entry name as table_id -- the value candidates.front() carried before the reordering.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Problem
#220 added a lazy-discovery guard to
LanceRestNamespaceDefaultGenerator::CreateDefaultEntry: before resolving a name, it checks membership inGetDefaultEntries()(alist_tablesround trip). The guard's purpose is sound — DuckDB probes the active catalog for system names (e.g.duckdb_tableswhenSHOW TABLESruns underUSE <lance_catalog>), and those probes must fall through to the system catalog instead of aborting the statement. But making a list round trip the gate for every point lookup has three defects:Stale credentials after secret rotation. The membership list runs with the ATTACH-time captured
bearer_token/api_key(GetDefaultEntries()uses the member fields), beforeResolveLanceNamespaceAuthre-resolves DuckDB secrets. After a secret rotation, point lookups of not-yet-materialized entries fail with stale credentials even though the describe/open path below — which does use freshly resolved credentials — would succeed.Already-qualified references are rejected. With a non-empty namespace id,
GetDefaultEntries()strips the namespace prefix from listed names, but the guard compares the raw entry name against the stripped list: a qualified reference (ns$tbl) is rejected by the guard before the candidate logic below (which deliberately describes already-prefixed names as-is) ever runs.List permission becomes a prerequisite for point lookups. Credentials that allow describe/open of a specific table but deny
list_tables, or tables intentionally omitted from listings, are misreported as not found.Fix: describe-first resolution
Drop the up-front membership gate; resolve auth first; describe the candidates with three-state outcomes — Found / NotFound / Error — where NotFound is a new typed signal instead of a guess:
Rust: new
ErrorCode::NamespaceTableNotFound, reported bydescribe_table_with_schema_innervia a classifier that only treats spec-conformant not-found as typed:lance_core::Error::NotFound, or aNamespaceError::TableNotFound/NamespaceNotFounddowncast out ofError::Namespace. The REST client materializes those variants from the numericcodefield of spec error bodies (NamespaceError::from_code); non-spec error bodies deserialize to other variants and deliberately remain generic describe errors.C++ candidates (canonical id = the namespace-qualified form, probed FIRST; the bare name is only an alias for server dialects that store unprefixed ids). On a hierarchical server a one-segment id addresses the ROOT namespace, so the alias is double-guarded:
C++ precedence on the canonical outcome:
CatalogSet::CreateDefaultEntriesnon-null contract for enumeration is preserved, and the real incompatibility surfaces at use time;nullptr: soft fall-through to the system catalog — the guard's original purpose;nullptr(that server's not-found dialect), listed → empty entry, and if the list itself fails the canonical candidate's describe error surfaces as anIOException— a possibly-existing table is never masqueraded as "not found".The thread-local FFI error is consumed on every path — including the slow fallback's failed-open paths — so it cannot leak into an unrelated
LanceFormatErrorSuffix()later.Trade-off (from review): on server dialects that store unprefixed ids, alias resolution now requires list permission to confirm membership; the canonical path still needs none. This is the price of not letting a hierarchical server's root namespace answer for an attached child namespace.
Also adds
rust/ffi/namespace.rstoRUST_FFI_DEPENDSin CMakeLists.txt — the hand-maintained dependency list predates the file, so incremental builds would not otherwise pick up this change.Error code numbering
ErrorCode55 is intentionally left unused: the in-flight #225 claims 55 for its own new code, and skipping it lets the two changes merge in either order without a value collision in the C++ mirror constant (the enum already tolerates a gap at 33).Tests
TableNotFounderror body (numeric code 4) must map to the typed FFI code, while an unreachable endpoint must stay a generic describe error.LANCE_TEST_NAMESPACEtests; the sqllogictest suite is unchanged from baseline (56/57 pass locally, the one failure —scan_limit_through_filter.test— is pre-existing on the base branch).Context
These defects predate and are independent of #225 (found while addressing its review feedback on a downstream port). Refs #220.