Skip to content

Cherry-pick apache/datafusion#25412 - #197

Merged
notfilippo merged 2 commits into
branch-55from
filippo.rossi/cherry-pick/apache-pr-25412-20261103
Nov 4, 2026
Merged

notfilippo merged 2 commits into
branch-55from
filippo.rossi/cherry-pick/apache-pr-25412-20261103

Conversation

@notfilippo

Copy link
Copy Markdown
Member

Cherry-picks apache#25412 onto branch-55 to fix duplicate-column errors in push_down_leaf_projections when merging projections with same-name aliases.

Includes apache#24242 as a prerequisite because it provides the passthrough_column helper required by the fix. Both commits retain their original authors, messages, and cherry-pick trailers.

cetra3 and others added 2 commits November 3, 2026 23:37
## Which issue does this PR close?

- Closes apache#24241

## Rationale for this change

This is a bugfix for a specific query shape.

## What changes are included in this PR?

Changes the way leaf expressions are extracted when the alias is
essentially the same as the column

## Are these changes tested?

Yes, with a test that fails on main currently

## Are there any user-facing changes?

Nope

(cherry picked from commit 0fc962f)
…xtraction projection (apache#25412)

## Which issue does this PR close?

None filed.

## Rationale for this change

This query fails to plan on `main`:

```sql
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

apache#25388 (draft) edits the same
arm of
`build_extraction_projection_impl`, for a different problem: duplicated
evaluation of `KeepInPlace` expressions,
apache#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.

apache#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:
apache#25459

---------

Co-authored-by: Claude Opus 5.5 <[email protected]>

(cherry picked from commit 47135be)
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-03T22:50:42.531543Z 7865139 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 78651394b6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@codecov-commenter

codecov-commenter commented Nov 3, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.00000% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.20%. Comparing base (3363f79) to head (7865139).

Files with missing lines Patch % Lines
...tafusion/optimizer/src/extract_leaf_expressions.rs 85.00% 4 Missing and 17 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           branch-55     #197      +/-   ##
=============================================
- Coverage      81.20%   81.20%   -0.01%     
=============================================
  Files           1111     1111              
  Lines         387808   387932     +124     
  Branches      387808   387932     +124     
=============================================
+ Hits          314925   315023      +98     
- Misses         54323    54332       +9     
- Partials       18560    18577      +17     

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

@notfilippo
notfilippo merged commit 9eda67e into branch-55 Nov 4, 2026
68 of 70 checks passed
@notfilippo
notfilippo deleted the filippo.rossi/cherry-pick/apache-pr-25412-20261103 branch November 4, 2026 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants