fix(search): send qualified table ids as multi-segment namespace paths in query_table - #228
Open
hellower wants to merge 2 commits into
Open
fix(search): send qualified table ids as multi-segment namespace paths in query_table#228hellower wants to merge 2 commits into
hellower wants to merge 2 commits into
Conversation
…s in query_table apply_base_request always built request.id as a single segment. Unlike GET-style operations (list_tables), whose URL path re-joins the id with the delimiter and is wire-identical either way, query_table POSTs the id array in the JSON request body, so the segment count is server-observable: a vector or full-text search on a multi-level table (parent$child$tbl) was sent as one segment and rejected by servers expecting three, even though describe/open/DDL were fixed by lance-format#220. Split the table id on the configured delimiter (default "$") for the REST namespace backend. Directory-namespace ids stay single-segment: directory namespaces are single-level and a directory name may legitimately contain '$'. Adds a wire-level test with a minimal in-process mock listener that only answers for the split 3-segment id and asserts the POSTed body id array. Also registers rust/ffi/query_table.rs in RUST_FFI_DEPENDS so incremental builds pick up edits to the file. Refs lance-format#217, lance-format#220.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3983a7bfd2
ℹ️ 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".
Document the lifetime and ownership invariants for the three unsafe blocks in the multi-level id wire test: the FFI call over the stack-allocated repr(C) config/options, the borrow of the transferred error-message allocation, and its single reclamation, per docs/rust_guidelines.md.
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
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.
Problem
apply_base_requestinrust/ffi/query_table.rsbuildsrequest.id = Some(vec![config.table_id.clone()])— always a single segment, even for a qualified id likeparent$child$tbl. Both namespace search paths (vector and full-text) flow through this one function:src/lance_search.cpppassescfg.table_idplus the configured delimiter intoLanceNamespaceSearchConfig.For most REST namespace operations a single pre-joined segment is harmless: GET-style operations such as
list_tablesjoin the id segments with the delimiter into the URL path, so one segment and a pre-split array are wire-identical.query_tableis different: the REST client POSTs the request as JSON and theidarray goes into the request body verbatim (lance-namespace-impls 6.0.0rest.rs::query_tableserializes the whole request via.json(&request)). The segment count is therefore server-observable: a vector or FTS search on a multi-level table is sent as one segment and rejected by servers expecting three, even though describe/open/DDL were fixed by #220.This is a remnant of the id-splitting defect family from #217: #220 fixed the describe/open/DDL paths but did not cover
query_table.Fix
Split the table id on the configured delimiter (default
$, matching the REST client) for the REST namespace backend. Directory-namespace ids deliberately stay single-segment: directory namespaces are single-level and a directory name may legitimately contain$.Also adds
rust/ffi/query_table.rstoRUST_FFI_DEPENDSin CMakeLists.txt — the hand-maintained dependency list predates this file, so incremental builds would not otherwise pick up this fix (or any other edit to the file).Test
Wire-level Rust test with a minimal self-contained in-process HTTP listener: the mock only answers requests whose POSTed JSON body carries the split 3-segment
idarray (anything else gets a spec-shapedTableNotFounderror body), and the test asserts both the percent-encoded joined id in the URL path and the split id array in the POST body, with the default$delimiter driving the split.Context
Found while porting the #225 review fixes to a downstream deployment; this defect exists on main independently of #225. Refs #217, #220.