adriangb opened a new pull request, #25478: URL: https://github.com/apache/datafusion/pull/25478
## Which issue does this PR close? - Closes https://github.com/apache/datafusion/issues/25446. Part of the leaf-pushdown EPIC: https://github.com/apache/datafusion/issues/25459 ## Rationale for this change A valid statement fails to plan. A sub-query projection renames a column to the name of a different column of the same input, and the query reads a struct field above it. `datafusion.optimizer.enable_leaf_expression_pushdown` is `true` by default, so this is a plain planning failure. ```sql create table t(a int, b int, s struct<x varchar>) as values (1, 10, {x: 'p'}), (2, 20, {x: 'q'}); -- 1. a rename beside a same-name alias, under a filter select b, a, s['x'] from (select t.a as b, t.b as a, s from t) where a > 0; -- 2. the same shape under a limit select b, a, s['x'] from (select t.a as b, t.b as a, s from t) limit 10; -- 3. a swap of two column names select a, c, s['x'] from (select t.b as a, t.a as c, s from t) where a > 0; ``` Observed on `main`, for all three: ```text Optimizer rule 'push_down_leaf_projections' failed caused by Schema error: Schema contains qualified field name t.a and unqualified field name a which would be ambiguous ``` Expected, and what you get with `set datafusion.optimizer.enable_leaf_expression_pushdown = false;`: ```text +---+----+--------+ +----+---+--------+ | b | a | t.s[x] | | a | c | t.s[x] | +---+----+--------+ +----+---+--------+ | 1 | 10 | p | | 10 | 1 | p | | 2 | 20 | q | | 20 | 2 | q | +---+----+--------+ +----+---+--------+ statements 1 and 2 statement 3 ``` `order by` and `group by` above the rename fail the same way. Query generators emit this shape. ## What changes are included in this PR? Both changes are in `datafusion/optimizer/src/extract_leaf_expressions.rs`. **1. The merge of the extraction projection.** `build_extraction_projection_impl` merges the extraction projection into the projection below it. For every column the parent needs, it resolved the name through the projection's rename map and then added the *input* column that the name resolves to. For `select t.a as b, t.b as a, s from t` the parent's `b` resolves to `t.a`, and `t.a` lands beside the output field `a` of the other rename. `Projection::try_new` rejects that schema. The merge keeps every expression of the projection below it, so the parent can still read each name that projection produces. Those names are now skipped instead of resolved, and a pass-through column whose name is one of the projection's output names is never added. A rename is a computed output, and the parent refers to it by its output name only. **2. The recovery check.** With the merge fixed, the shape reaches `split_and_push_projection`, which decided if it needs a recovery projection from the set of unqualified field names alone. Equal name sets do not prove equal values: the pushed plan exposes the table column `t.a` where the projection computed `t.a AS b`. Without this hunk the statements above return the columns swapped instead of an error. The recovery projection now also stays when a recovery expression is not a pass-through of a column. That second hunk is the same change as https://github.com/apache/datafusion/pull/25445, down to the text, so the two merge without a conflict. If that PR lands first, the hunk falls out of the rebase. **How this composes with the other open PRs on this function.** https://github.com/apache/datafusion/pull/25412 resolves both sides of the pass-through comparison against the input schema, and https://github.com/apache/datafusion/pull/25456 replaces the rename map with `ProjectionInliner`. This change sits above both: it decides which columns to add before either one resolves a name. I merged this branch into a local branch that holds both PRs. The single conflict is the loop header, which takes the guards of this PR around the body of the other two. The optimizer tests, the `struct`, `cse` and `push_down_filter` sqllogictest files, and the differential fuzz all pass there with the swap-alias shape of the fuzz generator turned on: 250 short cases, 125 short Parquet cases, 5000 extended cases and 1000 extended Parquet cases, 0 failures and 0 skips. ## What is the testing strategy for this PR? - `datafusion/sqllogictest/test_files/struct.slt`: the three statements above, plus the `order by` and `group by` variants, plus a swap where the struct field name is also a column name of the table, plus one `EXPLAIN` that shows the renames above the extraction projection and the struct field still read at the scan. - `datafusion/optimizer/src/extract_leaf_expressions.rs`: `test_extract_above_projection_that_swaps_column_names` covers the ambiguous schema. `test_extract_above_projection_that_redefines_column_name` covers the recovery projection that must keep a rename alive. - Three plan snapshots of neighbouring tests lose a duplicate pass-through column in the intermediate stage. The optimized plans of those three tests do not change. Commands and results: ```text cargo test --profile ci -p datafusion-optimizer 884, 26, 5 passed; 0 failed cargo test --profile ci -p datafusion-sqllogictest --test sqllogictests 520/520 files completed, 0 failures cargo clippy --profile ci -p datafusion-optimizer --all-targets -- -D warnings clean ``` ## Are there any user-facing changes? Statements of this shape plan and run. There is no API change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
