fix(unparser): scope a bounded EXISTS build side, so its limit selects rows - #201
Open
grokspice wants to merge 2 commits into
Open
fix(unparser): scope a bounded EXISTS build side, so its limit selects rows#201grokspice wants to merge 2 commits into
grokspice wants to merge 2 commits into
Conversation
…s rows A semi, anti or mark join unparses its build side as a correlated EXISTS subquery. A row bound on that side -- LIMIT, OFFSET, FETCH or LIMIT BY -- was emitted beside the correlation predicate in the subquery body, where SQL applies it after the WHERE. The bound then chose among the rows the correlation had already matched instead of choosing which rows the correlation could see, so the subquery searched the whole relation. That is a wrong-rows bug rather than a too-many-rows one: a semi or mark join reports a match on a row the plan never read, and an anti join drops a row it should have returned. Give the bounded body a scope of its own and correlate outside it, so the bound is applied first. The projection and DISTINCT rewrites move to the outer select with the correlation, because inside the bound both still decide which rows survive it. The scope has to keep answering to the name the correlation uses, which comes from the join keys rather than from the build side's schema -- a set operation's output carries no qualifier while the keys naming it still do. Where no single name works, the plan is left alone rather than emitting references that cannot bind: - the correlation names more than one of the build side's own inputs, so one scope cannot expose them all; or - its only qualifier is one the probe side also answers to, where renaming the scope would rebind the reference to the probe and compare the outer row with itself. Both keep today's output, which is wrong in the way this commit describes but does not additionally drop the correlation. Tests pin both declines. Also gives the set-operation wrapper an explicit wildcard projection: it previously relied on every caller overwriting the projection with SELECT 1, and a caller that wraps it in a further scope needs it to expose the body's columns to that scope. Fixes spiceai/spiceai#12595
Author
|
@copilot review |
An adversarial review of the previous commit found the self-join decline's justification wrong. The claim was that declining avoids dropping the correlation. It does not: the subquery's own FROM already shadows the outer relation of the same name, so both sides of the predicate bind to the inner one and the correlation is lost whatever the bound does. That shadowing predates this change and declining does not repair it -- what it buys is only that the scope is not also renamed out from under the reference, which would swap one wrong answer for a different one. Both decline snapshots now say they record known-incorrect output and point at spiceai/spiceai#12840, which tracks the qualifier rewrite that would let the unparser stop declining, so a reader does not read the snapshot as the intended result. No behaviour change.
There was a problem hiding this comment.
Pull request overview
Fixes bounded build-side handling when unparsing semi, anti, and mark joins as correlated EXISTS queries.
Changes:
- Detects row-bounding query clauses.
- Scopes bounded build queries before applying correlation predicates.
- Adds regression coverage for join and set-operation variants.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
datafusion/sql/src/unparser/ast.rs |
Adds row-bound detection. |
datafusion/sql/src/unparser/plan.rs |
Wraps bounded EXISTS build sides in derived scopes. |
datafusion/sql/tests/cases/plan_to_sql.rs |
Adds bounded-join regression tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // turning the correlation into a comparison of the outer row with | ||
| // itself. | ||
| [] if !names_a_probe_relation => Some("derived_limit".to_string()), | ||
| [relation] => Some(relation.table().to_string()), |
Comment on lines
+2425
to
+2447
| let probe_relations: Vec<&TableReference> = join | ||
| .left | ||
| .schema() | ||
| .iter() | ||
| .filter_map(|(relation, _)| relation) | ||
| .collect(); | ||
| let mut relations: Vec<TableReference> = Vec::new(); | ||
| let mut names_a_probe_relation = false; | ||
| let mut collect = |expr: &Expr| { | ||
| for column in expr.column_refs() { | ||
| let Some(relation) = &column.relation else { | ||
| continue; | ||
| }; | ||
| if probe_relations.contains(&relation) { | ||
| names_a_probe_relation = true; | ||
| } else if !relations.contains(relation) { | ||
| relations.push(relation.clone()); | ||
| } | ||
| } | ||
| }; | ||
| for (_, right) in &join.on { | ||
| collect(right); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
datafusion/sql/src/unparser/plan.rs:2430
- For
RightSemiandRightAnti, the caller swaps the effective probe/build plans atplan.rs:1011-1015, but this helper still treatsjoin.leftas the probe and the right member of eachonpair as the build key. With a bounded original-left side, this aliases the derived build table using the outer right table's name, leaving the left key unbound (and potentially shadowing the outer table). Determine the probe and build-key side from the join type, and add bounded right-semi/right-anti regressions.
let probe_relations: Vec<&TableReference> = join
.left
.schema()
.iter()
.filter_map(|(relation, _)| relation)
.collect();
datafusion/sql/src/unparser/plan.rs:2461
- This bare alias does not match the qualifier emitted by a dialect with
full_qualified_col(): a key such ascatalog.schema.t2.cremains fully qualified, while the derived table exposes onlyAS t2, so the new scoped path generates an unbindable predicate. This turns boundedEXISTSover qualified tables into invalid SQL. Rewrite build-side predicate qualifiers to the alias introduced here (while preserving probe references), or decline this scoping case until that rewrite is available; cover it with a fully-qualified-dialect test.
[relation] => Some(relation.table().to_string()),
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.
Summary
A semi, anti or mark join unparses its build side as a correlated
EXISTSsubquery. A row bound onthat side is emitted beside the correlation predicate in the subquery body, where SQL applies it
after the
WHERE— so the bound chooses among the rows the correlation already matched instead ofchoosing which rows the correlation may see, and the subquery searches the whole relation.
Before:
LIMIT 5after a correlated equality is a no-op: the body is non-empty whenever anyt2rowmatches, not only when one of the five rows the plan reads does.
After:
This is a wrong-rows bug rather than a too-many-rows one: a semi or mark join reports a match on a
row the plan never read, and an anti join is the mirror image and drops a row it should return.
Fixes spiceai/spiceai#12595.
Changes
QueryBuilder::bounds_rowsreports whether a query bounds which rows it returns —LIMIT,OFFSET,FETCHorLIMIT BY. All four are evaluated after the body'sWHERE, so all four havethe same problem;
OFFSETalone picks rows as much asLIMITdoes.build_exists_subquerygives a bounded body a scope of its own and correlates outside it. TheSELECT 1and drop-DISTINCTrewrites move to the outer select with the correlation, becauseinside the bound both the projection and
DISTINCTstill decide which rows survive it.wrap_setexpr_as_derived_selectgains an explicit wildcard projection and delegates to a newwrap_query_as_derived_select. It previously relied on every caller overwriting the projectionwith
SELECT 1; a caller that wraps it in a further scope needs it to expose the body's columns,and an unset projection renders as
SELECT FROM.Naming the scope
The scope has to keep answering to the name the correlation uses. That name comes from the join
keys, not from the build side's schema — a set operation's output carries no qualifier while the
keys naming it still do, so the schema would name the scope something nothing references.
Where no single name works, the plan is left alone rather than emitting references that cannot bind:
them all; or
reference to the probe and compare the outer row with itself — true for every row, which would
make
EXISTSfire whenever the bounded side is non-empty. That is strictly worse than the bugbeing fixed, so it declines.
Both declines keep today's output. That output is known-incorrect and the tests recording it say
so: the first still applies the bound after the correlation, and the second is already broken for an
unrelated reason -- the subquery's own
FROM "t"shadows the outer"t", so the correlation is lostto shadowing whatever the bound does. Declining does not repair either; it only avoids trading one
wrong answer for a different one.
Closing both needs the correlation's column qualifiers rewritten to the scope the derived table
introduces. That is filed as spiceai/spiceai#12840, together with the related bare-name-alias
limitation under a
with_full_qualified_col(true)dialect (spiceai/spiceai#12594), which this PRinherits from the unparser's existing aliasing convention and widens the reach of.
Test plan
Eight tests in
datafusion/sql/tests/cases/plan_to_sql.rs:..._left_semi_join_scopes_build_side_fetch— the reported shape...._left_anti_join_scopes_build_side_fetch— the mirror image...._left_mark_join_scopes_build_side_fetch— the same body reached through a projected boolean...._left_semi_join_scopes_build_side_limit_node— aLimitnode rather than a scanfetch,carrying an
OFFSETas well...._left_semi_join_scopes_bounded_set_operation_build_side— aUNIONbuild side, which isalready wrapped once, and asserts the inner select projects something.
..._left_semi_join_declines_multi_relation_build_side— pins the multi-relation decline...._left_semi_join_declines_probe_qualified_correlation— pins the self-join decline and assertsthe scope is not renamed.
..._left_semi_join_without_fetch_stays_flat— an unbounded build side keeps the flat body, sothe fix costs nothing when it is not needed.
The four bug-pinning tests were confirmed to fail with the fix disabled and pass with it.
cargo test -p datafusion-sql: 508 passed, 22 failed. The same 22 fail onspiceai-54atedd8861e6without this change (500 passed, 22 failed there) — they are pre-existing and unrelated.cargo clippy -p datafusion-sql --all-targetsis clean for the changed files.Note on formatting
spiceai-54is notcargo fmtclean atedd8861e6—datafusion/expr/src/expr.rs,datafusion/sql/src/unparser/{expr,utils}.rs,datafusion/core/tests/physical_optimizer/enforce_sorting.rsand
datafusion/sql/src/unparser/plan.rsall have drift under the pinned rustfmt (1.95.0, matchingrust-toolchain.toml). This PR leaves that drift alone so the diff stays reviewable; the code itadds is formatted.