fix: resolve pass-through columns against the input when merging an extraction projection

[adriangb]

Which issue does this PR close?

None filed.

Rationale for this change

This query fails to plan on main:

create table t(v int, s struct<a int>, env varchar) as values (1, {a: 10}, 'prod'), (2, {a: 20}, 'dev');

with samples as (
  select v, s, env from t
),
expanded as (
  select v, s, env from samples
  union all
  select v, s, env from samples where 1 = 2
)
select env, sum(s['a']) from expanded group by env;

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

enable_leaf_expression_pushdown is on by default, so this is a plain planning
failure for a valid statement. A union all whose one side is provably empty is
what 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_impl merges an extraction projection into the
projection 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_new rejects a schema
that holds samples.env and a bare env together.

This PR resolves both sides against the input schema before the comparison, and
pushes the column under the name the input gives it. A needed column that is not in
the input, or that is ambiguous there, is skipped. Before this change such a column
was added and Projection::try_new failed at the merge.
A same-name alias such as t.c AS c counts as a pass-through on the existing side too, the same as a bare t.c.

Are these changes tested?

Yes. The statement above is added to struct.slt, with a result check and an
EXPLAIN plan pin. It fails on main with the error above and passes with this change.

Three unit tests in extract_leaf_expressions.rs cover the merge directly. All three fail on main:

• test_merge_bare_column_into_qualified_projection: a filter names a over a projection that outputs test.a.
• test_merge_into_projection_with_same_name_alias: the projection spells the pass-through as test.a AS a.
• test_merge_appends_qualified_column_for_bare_request: the projection keeps only test.a and the merge needs a bare b. The appended column must be test.b. On main it is a bare b.

cargo test --workspace --exclude datafusion-sqllogictest passes: 126 suites,
12479 tests, 0 failures. The sqllogictest suite passes too: 520 files, 0
failures. cargo clippy -p datafusion-optimizer --all-targets and
cargo fmt --all -- --check are 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: duplicated
evaluation of KeepInPlace expressions, #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_needed loop.

#25282 also appends to the end of
struct.slt, as this PR does. Keeping both blocks is the whole resolution.

Part of the leaf-pushdown EPIC: #25459

[adriangb]

cc @pepijnve since you're working on these same files

[Copilot]

🟡 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.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 87.09677% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.84%. Comparing base (0566b55) to head (7e44ea3).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...tafusion/optimizer/src/extract_leaf_expressions.rs	87.09%	4 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25412      +/-   ##
==========================================
- Coverage   82.84%   82.84%   -0.01%     
==========================================
  Files        1149     1149              
  Lines      453357   453424      +67     
  Branches   453357   453424      +67     
==========================================
+ Hits       375597   375650      +53     
- Misses      54916    54921       +5     
- Partials    22844    22853       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[adriangb]

Summary

The fix is correct and small. I verified all claims in the description. I have 3 non-blocking suggestions (inline).

Root cause

flowchart TD
    A["Filter / Aggregate above<br/>needs: <code>env</code> (bare) or <code>samples.env</code>"] --> M{{"build_extraction_projection_impl<br/>merge into existing Projection"}}
    P["Existing Projection: <code>v, s, env</code> (bare)<br/>input: <code>samples.v, samples.s, samples.env</code>"] --> M
    M -->|main: compare raw Column values| X["<code>samples.env</code> != <code>env</code><br/>→ push again<br/>→ schema has <code>samples.env</code> AND <code>env</code><br/>❌ ambiguous"]
    M -->|PR: resolve both sides against input| O["both → <code>samples.env</code><br/>→ match, no push<br/>✅ plans"]

Loading

Verification

Check	Result
cargo test -p datafusion-optimizer --lib extract_leaf_expressions on PR head (61923e2)	✅ 57 passed
Same, with only the fix reverted (tests kept)	❌ 2 failed: both new tests
sqllogictests -- struct projection_pushdown on PR head	✅ pass
Simplified resolve_against (suggestion 1)	✅ 57 passed

Behavior change table

Needed column	Input schema	main	This PR
bare c	t.c	pushes bare c	pushes t.c (input spelling)
t.c	t.c, existing has bare c	pushes t.c → ambiguous	matched, no push
t.c	t.c, existing has t.c AS c	pushes t.c → ambiguous	matched, no push
any	column not in input (for example __common_expr_N)	skipped	skipped (unchanged)

Suggestions

#	Where	Kind	Blocking
1	resolve_against	Simplify to one lookup; fix doc claim about duplicate names	No
2	merge comment	Make shorter	No
3	struct.slt	Add EXPLAIN to lock the plan shape	No

The doc on resolve_against says a duplicate name "resolves to nothing". That is not always true. qualified_field_with_unqualified_name returns the one unqualified field when the name is also held by qualified fields (see dfschema.rs). The behavior is fine; only the doc is wrong. Suggestion 1 fixes both.

[adriangb]

@pepijnve if you are able to review this it would be great, we are hitting this in prod so pretty keen to fix it. I also think it's a good starting place to tackle the EPIC we have now.

[pepijnve]

nit: this sentence took me a couple of tries to grok. The function has an active name (an action), but the first sentence is describing a noun (a property). Might be better to write this in a 'Does computation xyz' style.

[pepijnve]

What are the 'sides' being referred to here? In the final sentence there's a reference to an empty union side. Makes this hard to interpret.

[kosiew]

@adriangb,

Thanks for the fix. The schema-based column resolution addresses the qualified/unqualified ambiguity, and the same-name alias handling covers the earlier concern. The focused optimizer and SQL regression tests passed. I have one optional test coverage suggestion below, which is not a blocker.

[kosiew]

Could we add a test where the existing projection retains only test.a and columns_needed requests bare b? It would verify that the newly appended column is test.b, with qualifier test and name b, rather than unqualified b. The current tests cover deduplication but not this specific case. This is optional and could be done in a follow-up.

[adriangb]

added in 68a2780

[adriangb]

Thanks @kosiew