hassaanch23 opened a new pull request, #25402: URL: https://github.com/apache/datafusion/pull/25402
## Which issue does this PR close? - Part of #25401. This fixes the `min`/`max` schema mismatch. The `avg`/`bit_*`/`stddev`/`var_*` panics have a different cause and will be a separate PR. ## Rationale for this change In a grouped query, an order-insensitive aggregate called with an `ORDER BY` fails whenever the plan runs in two phases, as it does with the default `target_partitions`: ```sql CREATE TABLE d (g INT, k INT, v INT) AS VALUES (1, 2, 20), (1, 1, 10), (2, 4, 40), (2, 3, 30); SELECT g, min(v ORDER BY k) FROM d GROUP BY g; -- Arrow error: Invalid argument error: number of columns(2) must match number of fields(3) in schema ``` `AggregateFunctionExpr::order_bys()` already returns no expressions for an order-insensitive aggregate, so its ORDER BY columns are never fed to the accumulator. `AggregateFunctionExpr::state_fields()` still passed `ordering_fields` in `StateFieldsArgs`, though. `Min` and `Max` don't override `state_fields`, and the default `AggregateUDFImpl::state_fields` appends `ordering_fields`. Their accumulators only emit the value, so the partial state schema declares fields that no column fills. `sum`, `count`, `bool_and` and `bool_or` are also order-insensitive, but they override `state_fields` without the ordering fields, so they already work. ## What changes are included in this PR? `AggregateFunctionExpr::state_fields()` now passes an empty `ordering_fields` when the aggregate is order-insensitive, mirroring `order_bys()`. Order-sensitive aggregates are unchanged. ## What is the testing strategy for this PR? A new query in `group_by.slt`, in the section that runs aggregators with `target_partitions = 8` (the plan is `Partial` → `FinalPartitioned`). It selects `MIN`, `MAX` and `SUM` with `ORDER BY ts DESC` over `sales_global`, with `SUM` as a control. Without the change it fails with `number of columns(4) must match number of fields(6)`; with the change it passes. The existing `SUM(amount ORDER BY ts DESC)` tests run as `mode=Single`, which is why they never reached this path. The other `.slt` files covering aggregates (`group_by`, `aggregate`, `array_agg`, `first_last_*`, `distinct_on`, `agg_func_substitute`, `window`, `order`) still pass. Checked by hand at the default `target_partitions`, the following now return the same rows as without the `ORDER BY`: integer and string `min`/`max`; `min` alongside `first_value(v ORDER BY k)`; and `min` alongside `sum` and `count`. ## Are there any user-facing changes? Queries that failed now return results. No API changes. ## Notes for reviewers The state field *names* for ordering fields are being reworked in #25196. This PR only changes *whether* order-insensitive aggregates receive `ordering_fields`, so the two shouldn't interact beyond a possible textual rebase. -- 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]
