Skip to content

fix(query): close the two ways an unresolved name still passed a select - #437

Merged
singaraiona merged 1 commit into
devfrom
fix/select-dist-nearest-labels
Aug 28, 2026
Merged

fix(query): close the two ways an unresolved name still passed a select#437
singaraiona merged 1 commit into
devfrom
fix/select-dist-nearest-labels

Conversation

@ser-vasilich

Copy link
Copy Markdown
Collaborator

Fixes #428

Already fixed, except for two ways around it

The report was filed against v2.5.15 and describes the same defect as #414, which has since been fixed: an unknown column reference now gets an invalid-scan marker that ray_execute rejects. Every shape in the report raises schema: column 'nosuch' not found on current dev.

Two ways around that marker survived, and both reproduce the report in full — a projection emitted under the wrong label, and an aggregate answering its neutral element. Neither should close as a duplicate.

Hole A — the _dist exemption was unconditional

_dist is exempt from the marker because a rerank produces that column while the query runs, so it is genuinely absent when the query compiles. But the exemption applied just as well to a query with no nearest: clause, where nothing will ever produce it. Verified on dev before the change:

(select {x: a d: _dist z: b from: T})  ->  a two-column table {x, d}
                                           where d holds b's data, z dropped
(select {s: (sum   _dist) from: T})    ->  0
(select {c: (count _dist) from: T})    ->  the operand's row count
(select {m: (max   _dist) from: T})    ->  INT64_MIN

The exemption is now conditional on the graph actually holding an OP_ANN_RERANK / OP_KNN_RERANK node. The rerank is built before projections compile, so the check is a walk over nodes already present, on the cold unknown-name path only.

Hole B — nearest: missing from the rename loop's skip list

The loop that names output columns is positional, so a clause keyword it fails to skip consumes an output slot and every name after it lands one column early. nearest: is counted out where the outputs are counted but was not skipped here, so a query naming it before its outputs labelled them one position off:

(select {from: TN nearest: (knn Vec [1.0 0.0]) i: id d: _dist take: 2})
  ->  columns labelled  nearest | i        instead of  i | d

Not changed

The marker-plus-validate_scan_columns split stays as it is. Raising at resolution time was considered and rejected for the reason the existing comment records: optimizer rewrites can legitimately make a scan dead, and an eager error would have to be threaded through every compile_expr_dag caller's NULL contract.

Tests

test/rfl/regress/issue_428.rfl covers both holes and the legitimate _dist case that must keep working. Confirmed red before the change: the projection shape returned a two-column table whose second column carried the third column's data under the second column's name.

No existing test depended on the silent behaviour — the assertions in this area all demand the error.

Suite: 3713/3713.

🤖 Generated with Claude Code

The general case was fixed with #414 — an unknown column reference gets
an invalid-scan marker that ray_execute rejects.  Two ways around it
survived, and both reproduced #428 in full: a projection emitted under
the wrong label, and an aggregate answering its neutral element.

`_dist` was exempt from the marker unconditionally.  The exemption is
needed — a rerank produces that column while the query runs, so it is
genuinely absent when the query compiles — but it applied just as well
where no rerank exists to produce it.  It is now conditional on the graph
actually holding one.

`nearest:` was missing from the skip list in the loop that names output
columns.  That walk is positional, so a keyword it fails to skip eats an
output slot and every name after it lands one column early.  It is
counted out where the outputs are counted, which is why only the naming
disagreed.

Both were verified to reproduce the report before the change: the
projection case returned a two-column table whose second column carried
the third column's data under the second column's name.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@singaraiona singaraiona left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with an empirical verification pass (head + base built ASan/UBSan, repros run on both). The fix is correct and does exactly what it says — both escape routes for an unresolved name are closed: the _dist exemption is now gated on a rerank node actually being present, and nearest_id is added to the output-naming skip list so it matches the output-count loop and select_output_count exactly. Suite 3713/3713 clean. Approving.

Verified, so reviewers don't re-chase them:

  • where: _dist is not a regression. (select {i: id from: TN where: (< _dist 0.5) nearest: (knn Vec [1.0 0.0]) take: 2}) fails with a schema error on both head and base — the WHERE reference compiles before the rerank node exists, so the exemption legitimately can't fire there. Head just gives a more specific message. Not newly broken.
  • The keyword sets now agree exactly, the keyword universe is closed at seven, and every other positional dict-walk that still omits nearest_id (7056/7341/7614/8147/8575/8818/2480/2561) is inside a by:-only path, unreachable today because nearest: + by: is rejected up front at query.c:5727.

One optional hardening (non-blocking): the regression test's [1 3] assertion rests on an exact floating-point tie — id 3 [1.0 1.0] and id 4 [0.5 0.5] have identical cosine distance to q=[1,0], and the result is decided only by the heap keeping the earlier-inserted row. On this platform the two distances are bit-identical at -O0 and -O2 (both reduce to correctly-rounded sqrt(0.5), so it's actually not FMA/libm-dependent for these inputs), so it passes robustly — but the test asserts a golden that rests on a tie rather than a real ordering, which is needlessly fragile for the next contributor. Changing id 4's vector to make the distances distinct (e.g. [0.5 0.5][0.9 0.1]) exercises the identical path and passes with zero golden edits — verified.

Two pre-existing follow-ups surfaced while tracing (neither introduced by this PR, both behave identically on base — worth a separate issue, not a blocker here):

  • The projection eval-fallback silently drops nearest: when there's no where:: (select {u: (distinct id) from: TN nearest: (knn Vec [1.0 0.0]) take: 2}) returns table-order rows, not nearest, with no error (confirmed same on head and base). distinct doesn't DAG-lower, so the fallback frees the graph including the rerank.
  • The exemption keys on graph state rather than the query — it works because outputs compile after the rerank node, but that coupling to compile order isn't enforced by anything. nearest_expr != NULL (already computed at query.c:5690) would be order-independent; worth considering if this area is touched again.

Approving and merging — I'll fold the test-tie tweak suggestion in as a comment rather than blocking on it.

@singaraiona
singaraiona merged commit a4e38f1 into dev Aug 28, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Engine bug — an unknown column name inside select resolves to nothing instead of raising, and the surviving projections shift under the wrong labels

2 participants