Repository navigation
Cherry-pick apache/datafusion#25412 - #197
Conversation
## 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)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 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 Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
Cherry-picks apache#25412 onto
branch-55to fix duplicate-column errors inpush_down_leaf_projectionswhen merging projections with same-name aliases.Includes apache#24242 as a prerequisite because it provides the
passthrough_columnhelper required by the fix. Both commits retain their original authors, messages, and cherry-pick trailers.