timsaucer opened a new pull request, #24887: URL: https://github.com/apache/datafusion/pull/24887
## Which issue does this PR close? - Closes #24884. ## Rationale for this change Using an aggregate UDAF as a window function (reachable through the DataFrame API) fails to plan whenever the physical optimizer decides to reverse the window to avoid an extra sort: ``` EnsureRequirements caused by Internal error: Assertion failed: col.name() == matching_name: Input field name first_value(?table?.v) ORDER BY [?table?.t ASC NULLS FIRST] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW does not match with the projection expression last_value(?table?.v) ORDER BY [?table?.t DESC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW. ``` `get_best_fitting_window` swaps in `WindowExpr::get_reverse_expr()`, and for aggregate-backed window expressions that reaches `AggregateFunctionExpr::reverse_expr`, which rewrites the aggregate's output *name* (`last_value(v) ORDER BY [t DESC]` → `first_value(v) ORDER BY [t ASC]`). A window exec derives its schema from `WindowExpr::field()`, so the node's output column gets renamed while the parent `ProjectionExec` still holds a `Column` with the old name. Renaming on reversal only makes sense for `AggregateExec`, which pins its schema at construction (`try_new` builds the schema *before* reversing exprs) — there the renamed name is a useful signal in `EXPLAIN` of which implementation actually runs, and no parent references it. The window path has the opposite requirement, which is why `WindowUDFExpr::reverse_expr` already carries `name` over unchanged. This makes the aggregate-backed window path behave the same way. The equivalent SQL query does not fail, because in SQL `last_value(v) OVER (...)` resolves to the `last_value` *window UDF*, which reverses without renaming. ### Scope Reviewed the rest of this bug class while here: - `first_value` ↔ `last_value` is the only `ReversedUDAF::Reversed` pair with a different name; `array_agg`, `string_agg` and `nth_value` reverse to themselves, so the rewrite is a no-op for them. - Two rules reach `get_best_fitting_window` — `enforce_sorting` and `enforce_distribution` — so the fix is applied at the `get_reverse_expr` level to cover both. - `OptimizeAggregateOrder` also calls `reverse_expr`, but `AggregateExec::with_new_aggr_exprs` keeps the original schema, so it cannot rename an output field. - `OptimizationInvariantChecker` does compare field names, but only for the **root** plan schema, so a rename on an intermediate node under a name-preserving projection is invisible to it. Hence the local assertion below. ## What changes are included in this PR? - `AggregateFunctionExpr::reverse_expr` is refactored into `reverse_expr_inner(preserve_name)`, with a new public `reverse_expr_preserving_name()`. Existing `reverse_expr` behavior is unchanged. - `PlainAggregateWindowExpr::get_reverse_expr` and `SlidingAggregateWindowExpr::get_reverse_expr` use the name-preserving variant. Their bodies were byte-identical, so they are factored into a shared `reverse_aggregate_window_expr` helper. - `get_best_fitting_window` now asserts that reversal did not change any output field name, so a future renaming `WindowExpr` implementation fails there — naming the culprit — instead of at a distant `ProjectionMapping` assertion. Note that the prefix-based `replace_fn_name_clause` also mangled user-supplied aliases beginning with `last_value`/`first_value` (e.g. an alias `last_value_desc` became `first_value_desc`); that is fixed on the window path too. ## What is the testing strategy for this PR? Two new regression tests, both verified to fail without the fix and pass with it: - `datafusion/core/tests/dataframe/mod.rs` — `window_reversal_preserves_output_field_names` reproduces the issue through the DataFrame API and reproduces the reported error exactly. It asserts on `create_physical_plan()` rather than `collect()`, because execution then hits the separate missing-`retract_batch` gap tracked by #24885; a comment marks where to upgrade the assertion once that lands. - `datafusion/core/tests/physical_optimizer/window_optimize.rs` — `test_window_reversal_preserves_output_field_names` builds two opposite-`ORDER BY` aggregate-UDAF windows under a projection, runs `EnsureRequirements`, and asserts the optimized plan's schema field names are unchanged. Without the fix it fails with `Input field name first_value_desc does not match with the projection expression last_value_desc`. The existing `test_reverse_expr_preserves_non_aliased_display_path` and the two neighboring `reverse_expr` display tests still pass, confirming the plain-aggregate renaming is untouched. All 510 sqllogictest files pass with no expectation changes — SQL cannot reach the renaming branch, and existing reversal expectations already show the original name with a reversed frame, which is exactly the shape this fix produces. Full extended suite (`--features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`): 11055 passed, 0 failed. `cargo clippy --all-targets --all-features -- -D warnings` clean. ## Are there any user-facing changes? Queries that previously failed to plan now plan successfully; no expected output or plan text changes for anything that worked before. `AggregateFunctionExpr::reverse_expr_preserving_name` is a new public method — additive, no breaking 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]
