Conversation
cd4c598 to
9943721
Compare
Derived-relation inference listed declared columns only. The response layer appends a synthetic distance cell, so s.distance failed 42703 on a closed schema. - append distance when ORDER BY carries vector_distance(...) - cover a hand-written ORDER BY vector_distance derived table with a vector index (numeric cells, nearest-first) and without one (numeric cells or a refusal; silent NULL is the only wrong outcome)
The filed case: a closed-schema source declares no distance column, so the synthetic cell resolves only when derived-relation inference adds the name. The open-schema form resolves with or without the fix, so the case pins the closed schema and reproduces the filed 42703.
The derived-relation check matched the literal name vector_distance. vector_cosine_distance, vector_neg_inner_product, and the operator forms the preprocessor rewrites to them also route an ORDER BY to a vector search whose rows carry a distance cell, so a closed-schema derived table over those forms still refused s.distance with 42703. - read the trigger from the function registry, the same source the order-by planner reads; no second name list
The declaration matched the literal vector_distance in the ORDER BY list only. SparseScore routes an ORDER BY to a search, a WHERE-clause vector trigger routes to the same plan, and every search answers with _surrogate beside distance; all three refused 42703 over a closed schema. - read both triggers from the registry the planner reads - scan the ORDER BY list and the WHERE clause - declare _surrogate next to distance
9943721 to
eeb56bb
Compare
farhan-syah
left a comment
There was a problem hiding this comment.
Request changes.
Checked statically against the branch ref (no checkout):
| Claim | Result |
|---|---|
cargo fmt --all -- --check clean |
Confirmed on every changed file. |
| Comment hygiene | One broken comment (inline). No issue numbers or links. |
| "every trigger and every shape" | Not true. The resolver re-derives routing from the AST and diverges from the planner in both directions (inline, blocker). |
| Commit list | The body cites cdb2e71a1, 4eddcfeb5, 235b7a28b, 994372149. The branch holds bec78889, 15cea2e, ca7baec, eeb56bb. Update the body after the rebase. |
Blockers are the layer finding, the broken comment, and the doc/code contradiction. The registry singleton and the tolerant tests are should-fix; both fall out of the layer change.
What landed correctly: the closed-schema wire case reproduces #305, and _surrogate is now declared beside distance.
| // without it `s.distance` and `s._surrogate` resolve against no relation | ||
| // and are refused with 42703, while the same projection over an | ||
| // open-schema source runs. | ||
| if order_by_triggers_a_search(query) || where_triggers_a_search(&query.body) { |
There was a problem hiding this comment.
Blocker: wrong layer. This re-derives the planner's routing from the AST. planner/select/derived_from.rs computes inner_plan = plan_query(subquery, …) and on the next line calls infer_subquery_relation with the AST instead of the plan. planner/select/entry.rs does the same for a CTE (cte_plan then infer_subquery_relation).
The plan already answers the question. The variants whose rows carry distance + _surrogate are the ones that lower to a vector-hit shape (exchange/resolve/exchange/post_process_arm.rs, classify_hit_shape): SqlPlan::VectorSearch | SparseSearch | MultiVectorSearch.
The AST copy diverges today:
| Query | Planner | This resolver | Result |
|---|---|---|---|
WHERE sparse_score(…) > 0.1 |
scalar fallback (where_search.rs, dispatch_trigger) |
declares | s.distance is NULL where main raised 42703 |
ORDER BY vector_distance(x) (one arg) |
Ok(None) (order_by/triggers.rs, args.len() < 2), scan |
declares | NULL |
WHERE multi_vector_search(…) |
SqlPlan::MultiVectorSearch (where_search.rs), vector-hit rows |
not declared | 42703 stays |
body whose FROM is not Scan / Join |
Ok(None) (try_extract_sort_search) |
declares | NULL |
The first two turn a loud error into a silent NULL: the same class #305 was filed against, one layer up.
End state: one predicate on SqlPlan (fn carries_search_cells(&self) -> bool matching those three variants). infer_subquery_relation takes the inner plan. expr_triggers_a_search, expr_contains_a_search, order_by_triggers_a_search, where_triggers_a_search go away. resolver/columns.rs (TableScope, the Derived arm) also infers without a plan and needs the same input.
| infer_body(catalog, &query.body) | ||
| let (mut columns, open) = infer_body(catalog, &query.body)?; | ||
|
|
||
| // The SEARCH preprocessor rewrites `SEARCH c USING VECTOR(...)` into |
There was a problem hiding this comment.
Blocker: broken comment. This sentence ends here. The next line starts a different one. Remove this line or finish it.
| }) | ||
| } | ||
|
|
||
| /// WHERE routing covers the vector trigger only: `where_search`'s dispatch |
There was a problem hiding this comment.
Blocker: doc contradicts code. "covers the vector trigger only", but expr_contains_a_search → expr_triggers_a_search matches SparseSearch too. The planner's WHERE dispatch sends SparseSearch to scalar evaluation, so the code is the wrong side of this contradiction, not the comment. Resolved by the layer change above.
|
|
||
| /// The process-wide built-in registry. Read-only after construction; used | ||
| /// where a function name alone decides routing. | ||
| pub fn builtin_registry() -> &'static FunctionRegistry { |
There was a problem hiding this comment.
Should-fix: third registry singleton. planner/const_fold.rs (DEFAULT_REGISTRY) and resolver/expr/functions.rs (FUNCTION_REGISTRY) already hold a LazyLock<FunctionRegistry>. The layer change removes this caller. If a static stays, keep one and point the other two at it.
| .unwrap_or_else(|e| panic!("a WHERE-clause trigger must declare s.distance: {e}")); | ||
| for row in &rows { | ||
| assert!( | ||
| row.is_empty() || row.parse::<f64>().is_ok(), |
There was a problem hiding this comment.
Should-fix: the test certifies a NULL cell. Once the declaration comes from the plan, a declared cell is always filled. Pin parse::<f64>() with no is_empty() escape. Same at the sparse case below.
| "distance must be numeric where filled: {rows:?}" | ||
| ), | ||
| Err(e) => assert!( | ||
| !e.contains("42703"), |
There was a problem hiding this comment.
Should-fix: the Err arm asserts nothing about the fix. Any error without 42703 passes. Same shape at the no-index case above (!e.to_string().is_empty()). With the plan-derived declaration this query either resolves with numeric cells or the sparse read refuses for a stated reason. Pin one outcome per test.
The derived-relation inference re-derived search routing from the AST and diverged from the planner in both directions: a WHERE sparse_score(...) or a one-argument ORDER BY vector_distance(...) declared a distance cell the planner never fills (silent NULL where main refused 42703), while SqlPlan::MultiVectorSearch went undeclared (42703 stays). SqlPlan::carries_search_cells() matches the three variants whose rows lower to the vector-hit shape; infer_subquery_relation reads the cells from the subquery's own plan, which the planner call sites already build. TableScope's derived arm plans non-lateral factors for the same input; a correlated LATERAL factor cannot be planned at scope time and routes no cells. The four AST heuristic functions and the third registry singleton are gone.
Summary
A derived relation over a closed-schema source refused
s.distancewith42703, while the same projection over an open-schema source ran. The response layer answers a search-shaped query with two synthetic cells the schema cannot name:distance(Float64) and_surrogate(the internal row id, resolved to the user primary key). The declaration follows the plan:SqlPlan::carries_search_cells()matches the three variants whose rows lower to the vector-hit shape (VectorSearch | SparseSearch | MultiVectorSearch), andinfer_subquery_relationreads the cells from the subquery's own plan — the source the planner already uses, never a second AST reading of it.Closes #305.
Behaviour changes
s.distanceover a closed-schema derived relation whose body routes to a search (ORDER BY vector_*and the operator forms<=>/<#>/<->,sparse_score, routingWHEREforms,WHERE multi_vector_search(...))42703s._surrogateover the same shapes42703ORDER BY vector_distance(..), a body whoseFROMis neitherScannorJoin)4270342703— a declared cell is never NULLRoot cause
Derived-relation inference (
resolver/derived.rs) re-derived "is this a search?" from the AST. The planner answers the same question inwhere_search/order_by/triggers, and the two diverged in both directions: declarations for shapes that route no search (silent NULL cells), and no declaration forSqlPlan::MultiVectorSearch.What changed
Commit
bec788896— the declaration: appenddistanceto a derived relation's columns when its body is search-shaped; unit tests cover declaration and non-declaration.Commit
15cea2eec— the filed case as a wire test: closed schema plus a vector index,SEARCH … USING VECTOR(…)in the derived body; the open-schema form never reproduced the refusal, which is why the case pins the closed schema.Commit
ca7baec72— the trigger set: the declaration readsSearchTriggerfrom the function registry, the same source the planner reads.Commit
eeb56bb4e—_surrogatedeclared besidedistance; the WHERE clause scanned.Commit
995215c8c— the layer move:SqlPlan::carries_search_cells()is the one predicate;infer_subquery_relationtakes the inner plan (derived_frompassesinner_plan,entrypassescte_plan, andTableScope's derived arm plans non-lateral factors for the same input — a correlatedLATERALfactor cannot be planned at scope time and routes no cells); the four AST heuristic functions and the third registry singleton are deleted.Regression proof
On
mainwithout the change, the reworked wire case fails 5 of 20 — including the filedSEARCH … USING VECTOR(…)case and the_surrogateprojection. On the branch head all 20 pass.Tested
cargo nextest run -p nodedb-sql --lib— 892 pass (derived-relation andcarries_search_cellsunit tests included)cargo nextest run -p nodedb --test wire -E 'test(cases::sql_search_subquery_composition)'— 20 pass; onmainwith the same tests: 15 pass / 5 failcargo fmt --all -- --check,cargo clippy -p nodedb-sql --all-targets,nodedb-preflight.sh: cleanNotes
synthetic_columndeclares the cells as nullableUnknowntype: the declaration resolves the name; the response layer supplies the value.WHERE multi_vector_search(...)plansSqlPlan::MultiVectorSearch, which the lowering step does not support yet (42601, variant named). The cell declaration is plan-derived, so that refusal is the honest one;42703no longer appears for it.order_by/triggersnoScan/Jointo read, so it routes no search; its projection refuses the cells.