Conversation
…xtraction projection
`build_extraction_projection_impl` merges an extraction projection into the
projection below it, and adds the pass-through columns that the merged
projection does not already carry. It compared the columns it was about to add
against the projection's own expressions without putting the two in the same
space.
A projection can list bare column names over a qualified input. Eliminating the
empty side of a union leaves exactly that. The comparison then misses, the
column is added a second time under its bare name, and `Projection::try_new`
rejects the result:
Optimizer rule 'push_down_leaf_projections' failed
caused by
Schema error: Schema contains qualified field name samples.env and
unqualified field name env which would be ambiguous
Resolve both sides against the input schema, and push the column under the name
the input gives it.
|
cc @pepijnve since you're working on these same files |
There was a problem hiding this comment.
🟡 Changes recommended
Same-name pass-through aliases remain excluded from deduplication and can recreate the ambiguous schema.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes projection merging to resolve pass-through columns against the input schema.
Changes:
- Normalizes column qualifiers before deduplication.
- Adds an SQL regression test for the planning failure.
File summaries
| File | Description |
|---|---|
extract_leaf_expressions.rs |
Resolves merged pass-through columns against input schemas. |
struct.slt |
Tests the previously failing union query. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ion projection A projection can spell a pass-through column as `t.c AS c`. The merge only counted bare column expressions as existing pass-throughs, so it added `t.c` beside the `c` the alias already outputs, which is an ambiguous schema. Use `passthrough_column` to collect them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25412 +/- ##
========================================
Coverage 82.32% 82.33%
========================================
Files 1137 1137
Lines 431824 431953 +129
Branches 431824 431953 +129
========================================
+ Hits 355500 355637 +137
+ Misses 54838 54817 -21
- Partials 21486 21499 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Which issue does this PR close?
None filed.
Rationale for this change
This query fails to plan on
main:enable_leaf_expression_pushdownis on by default, so this is a plain planningfailure for a valid statement. A
union allwhose one side is provably empty iswhat produces the shape, and query generators emit that: a dashboard panel adds a
second branch behind a comparison of two constants and the panel stops working.
What changes are included in this PR?
build_extraction_projection_implmerges an extraction projection into theprojection below it, and then adds the pass-through columns the merged projection
does not already carry. It compared the columns it was about to add against the
projection's own expressions without putting the two in the same space.
A projection can list bare column names over a qualified input. Removing the empty
side of the union leaves exactly that. The comparison then misses, the column is
added a second time under its bare name, and
Projection::try_newrejects a schemathat holds
samples.envand a bareenvtogether.This PR resolves both sides against the input schema before the comparison, and
pushes the column under the name the input gives it. A name the input schema holds
more than once resolves to nothing, because no single spelling is correct there.
A same-name alias such as
t.c AS ccounts as a pass-through on the existing side too, the same as a baret.c.Are these changes tested?
Yes. The statement above is added to
struct.slt. It fails onmainwith theerror above and passes with this change.
Two unit tests in
extract_leaf_expressions.rscover the merge directly. Both fail onmainwith an ambiguoustest.aerror:test_merge_bare_column_into_qualified_projection: a filter namesaover a projection that outputstest.a.test_merge_into_projection_with_same_name_alias: the projection spells the pass-through astest.a AS a.cargo test --workspace --exclude datafusion-sqllogictestpasses: 126 suites,12479 tests, 0 failures. The sqllogictest suite passes too: 520 files, 0
failures.
cargo clippy -p datafusion-optimizer --all-targetsandcargo fmt --all -- --checkare clean.Are there any user-facing changes?
A statement of this shape plans instead of failing. No API change.
Note on overlapping work
#25388 (draft) edits the same arm of
build_extraction_projection_impl, for a different problem: duplicatedevaluation of
KeepInPlaceexpressions, #25329.It does not touch the qualifier comparison this PR changes, so the two are
independent in behaviour, but whichever lands second will need a small rebase at
the tail of the
columns_neededloop.#25282 also appends to the end of
struct.slt, as this PR does. Keeping both blocks is the whole resolution.