hassaanch23 opened a new pull request, #25403: URL: https://github.com/apache/datafusion/pull/25403
## Which issue does this PR close? - Part of #25401. This fixes the panics. The `min`/`max` schema mismatch is fixed separately in #25402, and the two PRs are independent. ## Rationale for this change These aggregates panic when called with an `ORDER BY`, grouped or not, and on any number of partitions: ```sql CREATE TABLE d (g INT, k INT, v INT) AS VALUES (1, 2, 6), (1, 1, 3), (2, 4, 12), (2, 3, 10); SELECT g, avg(v ORDER BY k) FROM d GROUP BY g; -- panicked at datafusion/functions-aggregate/src/average.rs:1101:9: -- assertion `left == right` failed: single argument to update_batch -- left: 2 -- right: 1 ``` The same assertion fires for `bit_and`, `bit_or` and `bit_xor` (`prim_op.rs:98`), and for `stddev`, `stddev_pop`, `var_samp` and `var_pop` (`variance.rs:537`). None of these declares an `order_sensitivity`, so each gets the default `AggregateOrderSensitivity::HardRequirement`. `AggregateFunctionExpr::order_bys()` therefore returns their `ORDER BY` expressions, which are passed to the accumulator as extra input columns, and each accumulator asserts that it receives exactly one. ## What changes are included in this PR? `Avg`, `BitwiseOperation`, `Stddev`, `StddevPop`, `VarianceSample` and `VariancePopulation` now return `AggregateOrderSensitivity::Insensitive`, as `Sum` and `Count` already do. Their results don't depend on input order, so the `ORDER BY` is ignored and no longer fed to the accumulator. Their `state_fields` implementations already exclude ordering fields, so no other change is needed. ## What is the testing strategy for this PR? A new query in `group_by.slt`, next to the existing `SUM(amount ORDER BY ts DESC)` test, runs all eight functions with an `ORDER BY` over a small table. The expected values were computed by hand; for example, the group with values 3 and 6 gives `bit_and` 2, `bit_or` 7, `bit_xor` 5, `var_samp` 4.5 and `var_pop` 2.25. Without the change the query panics; with it, it passes. The other `.slt` files covering aggregates (`group_by`, `aggregate`, `array_agg`, `first_last_*`, `distinct_on`, `agg_func_substitute`, `window`, `order`) still pass. I also ran each function with and without `ORDER BY`, grouped at the default `target_partitions`, grouped with `target_partitions = 1`, and ungrouped. All 24 combinations return identical results. ## Are there any user-facing changes? Queries that panicked now return results. No API changes. ## Notes for reviewers - **Left as they are:** `corr`, `covar_*`, `regr_*`, `median` and `approx_distinct`/`approx_median` also keep the default `HardRequirement`. They tolerate the extra input column and return correct results, so this PR doesn't touch them. - **Deliberately excluded:** the `percentile_cont`/`approx_percentile_cont` family, because their `WITHIN GROUP (ORDER BY ...)` carries the value argument. -- 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]
