fix(substrait): Correctly parse field references in subqueries - #20439
Merged
Conversation
neilconway
changed the title
neilconway
force-pushed
the
neilc/substrait-fix-subquery
branch
from
February 20, 2026 15:02
e100c43 to
d0437e0
Compare
Contributor
Author
|
Hi @gabotechs @waynexia Would you have time to take a quick look at this when you get a chance? Thank you! |
Contributor
|
Requesting some backup for reviewing this one now. Thanks for taking the time! |
benbellick
approved these changes
Feb 26, 2026
benbellick
left a comment
benbellick
left a comment
Contributor
There was a problem hiding this comment.
👋 dropping in from the substrait side of things as @gabotechs's backup! It all looks good to me. Just left two comments, one about something stylistic and the other about another test that could be good, but feel free to take it or leave it. Thanks! 🚀
gabotechs
approved these changes
Feb 27, 2026
gabotechs
left a comment
gabotechs
left a comment
Contributor
There was a problem hiding this comment.
This looks good to me, code looks correct and well tested 👍 thanks @neilconway for the quality PR! and @benbellick for the review 🙇
Contributor
Author
|
Thanks @benbellick @gabotechs ! |
de-bgunter
pushed a commit
to de-bgunter/datafusion
that referenced
this pull request
Mar 24, 2026
…e#20439) ## Which issue does this PR close? - Closes apache#20438. ## Rationale for this change The substrait consumer parsed field references in correlated subqueries incorrectly. Field references were always resolved relative to the schema of the current (innermost) subquery, leading to incorrect results. ## What changes are included in this PR? We now maintain a stack of outer query schemas, and pushes/pops elements from it as we traverse subqueries. When resolving field references, we now use `FieldReference.root_type` to detect outer query field references and resolve them against the appropriate schema. This commit updates the expected results for parsing TPC-H queries, because several of them were parsed incorrectly (the misparsing was probably not detected because the incorrect parse didn't result in any illegal queries, by sheer luck). This also means we can enable Q17, which failed to parse before. ## Are these changes tested? Yes. Test results updated to reflect new, correct behavior, and new unit tests added. ## Are there any user-facing changes? The behavior of the substrait consumer has changed, although the previous behavior was wrong and it seems a bit unlikely anyone would have dependend on it. The `DefaultSubstraitConsumer` API is slightly changed (new private field).
LiaCastaneda
pushed a commit
to DataDog/datafusion
that referenced
this pull request
Mar 30, 2026
…e#20439) - Closes apache#20438. The substrait consumer parsed field references in correlated subqueries incorrectly. Field references were always resolved relative to the schema of the current (innermost) subquery, leading to incorrect results. We now maintain a stack of outer query schemas, and pushes/pops elements from it as we traverse subqueries. When resolving field references, we now use `FieldReference.root_type` to detect outer query field references and resolve them against the appropriate schema. This commit updates the expected results for parsing TPC-H queries, because several of them were parsed incorrectly (the misparsing was probably not detected because the incorrect parse didn't result in any illegal queries, by sheer luck). This also means we can enable Q17, which failed to parse before. Yes. Test results updated to reflect new, correct behavior, and new unit tests added. The behavior of the substrait consumer has changed, although the previous behavior was wrong and it seems a bit unlikely anyone would have dependend on it. The `DefaultSubstraitConsumer` API is slightly changed (new private field).
Smallfu666
pushed a commit
to Smallfu666/datafusion
that referenced
this pull request
Aug 18, 2026
…field references (apache#23488) ## Which issue does this PR close? - Closes apache#16280. ## Rationale for this change The Substrait producer errors on `Expr::OuterReferenceColumn`, so any plan containing a correlated subquery cannot be serialized currently. DataFusion's round-trip tests don't hit this because the optimizer decorrelates subqueries into joins before serialization, but any workflow that serializes *unoptimized* plans (e.g. sending raw plans between systems for later optimization in my case) fails on queries like TPC-H q2/q4/q17/q20/q21/q22, and ~26 cases in `joins.slt` fail in `--substrait-round-trip` mode. A previous attempt (apache#18987) was closed because it introduced a non-standard mechanism for outer references, producing plans only DataFusion would be able consume but Substrait already represents correlated references natively via a `FieldReference` with an `OuterReference` root type and a `steps_out` depth. The consumer side of was implemented in apache#20439, which resolves `OuterReference` field references against a stack of outer schemas. This PR implements the producing half, symmetric with that design, so correlated plans round-trip using only standard Substrait. ## What changes are included in this PR? - `SubstraitProducer` gains outer-schema-stack methods mirroring `SubstraitConsumer`: `push_outer_schema` / `pop_outer_schema` (default no-ops) and `get_outer_schema(steps_out)` (default `None`), plus a `handle_outer_reference_column` method so custom producers can override the behaviour like every other expression kind. Defaults are backward compatible: existing custom producers are unaffected unless a plan actually contains an outer reference, in which case they now get an actionable error instead of `not_impl_err`. - `DefaultSubstraitProducer` maintains the stack in a `Vec<DFSchemaRef>`. - The four subquery producers (`from_in_subquery`, `from_scalar_subquery`, `from_exists`, `from_set_comparison`) push the enclosing query's schema around the subquery plan conversion (via a shared `produce_subquery_rel`, analogous to the consumer's `consume_subquery_rel`). - `from_outer_reference_column` (previously unused and emitting an incorrect plain `RootReference`) now resolves the column against the outer-schema stack, innermost first, and emits a `FieldReference` with an `OuterReference` root and the corresponding `steps_out`. - `to_substrait_rex` dispatches `Expr::OuterReferenceColumn` to the new handler instead of erroring. ## Are these changes tested? Yes: - New round-trip tests in `roundtrip_logical_plan.rs` covering correlated `EXISTS`, correlated `IN` subquery, correlated scalar subquery, and a nested correlated subquery that crosses two subquery boundaries (`steps_out = 2`). Each test asserts the produced plan contains an `OuterReference` at the expected depth and that the plan round-trips through the existing consumer with its schema intact. - Consumer-side resolution was already covered by the tests added in apache#20439; these tests now exercise both halves together. - `joins.slt` in `--substrait-round-trip` mode goes from 38 failures to 12; the remaining failures are pre-existing gaps unrelated to outer references (`USING` join constraint, plan-level lateral `LogicalPlan::Subquery`, duplicate unqualified field names). ## Are there any user-facing changes? - Plans containing correlated subqueries now serialize instead of returning "not implemented", emitting spec-standard `OuterReference` field references. - `SubstraitProducer` has three new provided methods and `handle_outer_reference_column`; all have defaults, so existing implementations continue to compile. - The signature of the public helper `from_outer_reference_column` changed (it now takes the producer and the outer field) — its previous form resolved against the wrong schema and emitted a plain `RootReference`, and it was not called from anywhere in the crate.
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
3 participants
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.
Which issue does this PR close?
Rationale for this change
The substrait consumer parsed field references in correlated subqueries incorrectly. Field references were always resolved relative to the schema of the current (innermost) subquery, leading to incorrect results.
What changes are included in this PR?
We now maintain a stack of outer query schemas, and pushes/pops elements from it as we traverse subqueries. When resolving field references, we now use
FieldReference.root_typeto detect outer query field references and resolve them against the appropriate schema.This commit updates the expected results for parsing TPC-H queries, because several of them were parsed incorrectly (the misparsing was probably not detected because the incorrect parse didn't result in any illegal queries, by sheer luck). This also means we can enable Q17, which failed to parse before.
Are these changes tested?
Yes. Test results updated to reflect new, correct behavior, and new unit tests added.
Are there any user-facing changes?
The behavior of the substrait consumer has changed, although the previous behavior was wrong and it seems a bit unlikely anyone would have dependend on it. The
DefaultSubstraitConsumerAPI is slightly changed (new private field).