hassaanch23 commented on code in PR #25403: URL: https://github.com/apache/datafusion/pull/25403#discussion_r4166830482
########## datafusion/sqllogictest/test_files/group_by.slt: ########## @@ -2927,6 +2927,31 @@ GRC 110 [30.0, 80.0] FRA 250 [50.0, 200.0] TUR 175 [75.0, 100.0] +# ORDER BY is ignored by order-insensitive aggregators. These aggregators used +# to panic because their ORDER BY expressions were passed to the accumulator as +# extra input columns (issue #25401). +statement ok +CREATE TABLE insensitive_order_by (g INT, k INT, v INT) AS VALUES + (1, 2, 6), (1, 1, 3), (2, 4, 12), (2, 3, 10); + +query IRIIIRRRR Review Comment: Thanks for the review! Added in 31fa60f35: the same eight aggregates without `GROUP BY`, with `WHERE v > 0` so the filter splits the input across partitions and the query runs as a Partial and a Final aggregate. While doing it, I found that this query already passes on `main`, in both single-phase and two-phase plans. The extra ORDER BY column does reach the ungrouped accumulators too, but they only read `values[0]` (e.g. `AvgAccumulator::update_batch`). The groups accumulators instead assert a single argument (`assert_eq!(values.len(), 1, ...)` in `AvgGroupsAccumulator::update_batch`), and that's where the grouped queries panicked. So the new query guards the ungrouped path against a regression rather than reproducing a failure. -- 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]
