fix: mark avg, bit_and/or/xor, stddev and variance as order-insensitive by hassaanch23 · Pull Request #25403 · apache/datafusion

hassaanch23 · 2026-09-17T09:16:26Z

Which issue does this PR close?

Part of Order-insensitive aggregates with an ORDER BY fail in grouped queries (min/max schema mismatch, avg/bit_*/stddev/var panic) #25401. This fixes the panics. The min/max schema mismatch is fixed separately in fix: omit ordering fields from the state of order-insensitive aggregates #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:

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.

Calling avg, bit_and, bit_or, bit_xor, stddev, stddev_pop, var_samp or var_pop with an ORDER BY panicked with "single argument to update_batch". These functions did not declare an order sensitivity, so they got the default HardRequirement. AggregateFunctionExpr::order_bys() then returned their ORDER BY expressions, which were passed to the accumulator as extra input columns, and each accumulator asserts it receives exactly one. Their results do not depend on input order, so declare them Insensitive, as sum and count already are. The ORDER BY is then ignored. Part of apache#25401

codecov-commenter · 2026-09-17T13:45:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.28%. Comparing base (e5469e1) to head (ba8233c).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25403      +/-   ##
==========================================
- Coverage   82.28%   82.28%   -0.01%     
==========================================
  Files        1137     1137              
  Lines      430211   430229      +18     
  Branches   430211   430229      +18     
==========================================
+ Hits       354003   354015      +12     
- Misses      54784    54789       +5     
- Partials    21424    21425       +1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…regates-with-order-by

…tes (apache#25402) ## Which issue does this PR close? - Part of apache#25401. This fixes the `min`/`max` schema mismatch. The `avg`/`bit_*`/`stddev`/`var_*` panics have a different cause and are fixed in apache#25403; the two PRs are independent. ## Rationale for this change An order-insensitive aggregate called with an `ORDER BY` fails whenever the aggregation runs in two phases (`Partial` → `Final`/`FinalPartitioned`), grouped or not: ```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 ``` `AggregateExprBuilder::build()` derived the aggregate's `ordering_fields` from its `ORDER BY` even when the function is order-insensitive. `AggregateFunctionExpr::order_bys()` hides those expressions from the accumulator, but `state_fields()` still passed `ordering_fields`. `Min` and `Max` use the default `AggregateUDFImpl::state_fields`, which appends them, while their accumulators only emit the value. The partial state schema therefore declared fields that no column filled. `sum`, `count`, `bool_and` and `bool_or` override `state_fields` without ordering fields, so they already worked. ## What changes are included in this PR? `AggregateExprBuilder::build()` now drops `order_bys` when the function's `order_sensitivity()` is insensitive. `ordering_fields` is then empty from the start, so the aggregate carries no ordering state that later code has to ignore. Order-sensitive aggregates are unchanged. None of the built-in order-insensitive aggregates reads `order_bys` from `AccumulatorArgs`, and plans display aggregates by `name()`, so EXPLAIN output does not change. ## What is the testing strategy for this PR? Two queries in `group_by.slt`, in the section that sets `target_partitions = 8`, each selecting `MIN`, `MAX` and `SUM` with `ORDER BY ts DESC` over `sales_global`, with `SUM` as a control: - Grouped by `country`. The plan is `Partial` → `FinalPartitioned`. On `main` it fails with `number of columns(4) must match number of fields(6)`. - Ungrouped, with `WHERE amount > 0` so the input is multi-partition and the plan is `Partial` → `Final`. On `main` it fails with `number of columns(3) must match number of fields(5)`. Without the filter, the single-partition input runs in `Single` mode and never builds a partial state. Both queries pass with this change. The other `.slt` files covering aggregates still pass (`group_by`, `aggregate`, `array_agg`, `first_last_*`, `distinct_on`, `agg_func_substitute`, `window`, `order`). ## 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 apache#25196. This PR only changes whether order-insensitive aggregates have ordering fields at all, so the two shouldn't interact beyond a possible textual rebase. --------- Co-authored-by: Neil Conway <neil.conway@gmail.com>

alamb · 2026-09-17T16:19:49Z

I verified this happens in both DF 55 and on main:

andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ datafusion-cli
DataFusion CLI v55.1.0
> 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;
0 row(s) fetched.
Elapsed 0.034 seconds.


thread 'tokio-rt-worker' (17694786) panicked at datafusion/functions-aggregate/src/average.rs:1101:9:
assertion `left == right` failed: single argument to update_batch
  left: 2
 right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Join Error
caused by
External error: task 41 panicked with message "assertion `left == right` failed: single argument to update_batch\n  left: 2\n right: 1"
>
\q
andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ ~/Software/datafusion-cli/datafusion-cli-55.0.0
DataFusion CLI v55.0.0
> 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;
0 row(s) fetched.
Elapsed 0.010 seconds.
thread 'tokio-rt-worker' (17695424) panicked at datafusion/functions-aggregate/src/average.rs:978:9:
assertion `left == right` failed: single argument to update_batch
  left: 2
 right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Join Error
caused by
External error: task 41 panicked with message "assertion `left == right` failed: single argument to update_batch\n  left: 2\n right: 1"
>
\q

alamb · 2026-09-17T16:21:07Z

I checked it all the way back to 52:

andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ ~/Software/datafusion-cli/datafusion-cli-54.0.0 -f  /Users/andrewlamb/Downloads/foo.sql
DataFusion CLI v54.0.0
0 row(s) fetched.
Elapsed 0.007 seconds.


thread 'tokio-rt-worker' (17697794) panicked at datafusion/functions-aggregate/src/average.rs:827:9:
assertion `left == right` failed: single argument to update_batch
  left: 2
 right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Join Error
caused by
External error: task 42 panicked with message "assertion `left == right` failed: single argument to update_batch\n  left: 2\n right: 1"
andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ ~/Software/datafusion-cli/datafusion-cli-53.0.0 -f  /Users/andrewlamb/Downloads/foo.sql
DataFusion CLI v53.0.0
0 row(s) fetched.
Elapsed 0.007 seconds.


thread 'tokio-rt-worker' (17697933) panicked at datafusion/functions-aggregate/src/average.rs:812:9:
assertion `left == right` failed: single argument to update_batch
  left: 2
 right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Join Error
caused by
External error: task 42 panicked with message "assertion `left == right` failed: single argument to update_batch\n  left: 2\n right: 1"
andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ ~/Software/datafusion-cli/datafusion-cli-52.0.0 -f  /Users/andrewlamb/Downloads/foo.sql
DataFusion CLI v52.0.0
0 row(s) fetched.
Elapsed 0.007 seconds.


thread 'tokio-runtime-worker' (17698023) panicked at datafusion/functions-aggregate/src/average.rs:812:9:
assertion `left == right` failed: single argument to update_batch
  left: 2
 right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Join Error
caused by
External error: task 42 panicked with message "assertion `left == right` failed: single argument to update_batch\n  left: 2\n right: 1"

github-actions Bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Sep 17, 2026

hassaanch23 mentioned this pull request Sep 17, 2026

fix: omit ordering fields from the state of order-insensitive aggregates #25402

Merged

Merge remote-tracking branch 'upstream/main' into fix/insensitive-agg…

9850945

…regates-with-order-by

Navigation Menu

fix: mark avg, bit_and/or/xor, stddev and variance as order-insensitive - #25403

fix: mark avg, bit_and/or/xor, stddev and variance as order-insensitive#25403
hassaanch23 wants to merge 2 commits into
apache:mainfrom
hassaanch23:fix/insensitive-aggregates-with-order-by

hassaanch23 commented Sep 17, 2026

Uh oh!

codecov-commenter commented Sep 17, 2026

Uh oh!

alamb commented Sep 17, 2026 •

edited

Loading

Uh oh!

alamb commented Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hassaanch23 commented Sep 17, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Notes for reviewers

Uh oh!

codecov-commenter commented Sep 17, 2026

Codecov Report

Uh oh!

alamb commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alamb commented Sep 17, 2026 •

edited

Loading