cetra3 opened a new pull request, #26397: URL: https://github.com/apache/datafusion/pull/26397
## Which issue does this PR close? - Closes #26391. ## Rationale for this change A query with a filter and a sort on an expression over a struct field can fail planning with a `SanityCheckPlan` error. `EnsureRequirements` removes a sort key that the filter makes constant, and after `ProjectionPushdown` merges two projections the plan no longer knows that the key is constant. See #26391 for a `datafusion-cli` reproduction and a step-by-step trace. The cause is in `EquivalenceGroup::project`. When a projection names one expression twice, under two aliases, an equivalence class is recorded with the first alias only. A projection source that reads the second alias is equal to a member of that class only after normalization through the equivalence classes, but `augment_projection_mapping` looked up the class of the source as written. The class, and the constant it carries, was then dropped. ## What changes are included in this PR? `augment_projection_mapping` (`datafusion/physical-expr/src/equivalence/class.rs`) falls back to the class of the normalized source when the source as written is in no class. - It still tries the source as written first. `normalize_expr` rewrites bottom-up, so an expression that is itself a class member can normalize to a form that is not, and replacing the first lookup could lose matches that work today. - It normalizes only expressions with children. A leaf that is in no class normalizes to itself, so most mapping entries (plain columns) cost nothing extra. #26391 also names the `self.is_expr_constant(source)` lookup in `project`. This PR leaves it unchanged. The constant in #26391 is an equivalence class that holds a literal, and the change above carries that class through the projection. A fallback in `is_expr_constant` would also cover a constant without a literal value, but it made `physical_plan_tpcds_all` about 4% slower, and no query I found needs it. ## What is the testing strategy for this PR? - `projection_pushdown.slt`: the query from #26391, with a result assertion. It fails with the `SanityCheckPlan` error without this change. - `test_project_constant_of_equivalent_source` and `test_project_class_of_equivalent_source` (`class.rs`): a constant class and a non-constant class that a projection source reaches only through `a == b`. Both fail without this change. `sql_planner` benchmarks against `main`: | Benchmark | Change | |---|---| | `physical_select_one_from_700` | −0.7% | | `physical_select_all_from_1000` | −0.6% | | `physical_plan_struct_join_agg_sort` | +0.8% | | `physical_plan_tpch_all` | +2.4% | | `physical_plan_tpcds_all` | +0.8% | Two runs of `main` on the same machine differed by up to 2%, so I read these as noise. ## Are there any user-facing changes? The query shape in #26391 plans and runs. Some plans can drop sort keys that are now known to be constant. No public API changes. 🤖 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]
