ranflarion opened a new issue, #24735: URL: https://github.com/apache/datafusion/issues/24735
## Describe the bug The `aggregation_time` metric on `GroupedHashAggregateStream` is added inside the per-accumulator loop, so each accumulator re-adds the elapsed time of every accumulator that ran before it in the same batch: https://github.com/apache/datafusion/blob/ee59f628b/datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs#L947-L983 `agg_start_time` is taken once before the loop over accumulators, and `add_elapsed(agg_start_time)` runs once per accumulator, so with N aggregate functions the metric accumulates `sum_{k=1..N} (t_1 + ... + t_k)` instead of `t_1 + ... + t_N` — an inflation of up to (N+1)/2. Plans with a single aggregate function are unaffected, which is why this is easy to miss on TPC-style benchmarks. Observed on a 28-function GROUP BY (an aggregation-heavy production workload): a single task reported 4m22s of `aggregation_time` inside a 2m0s stage, and the operator's summed metric read 58m33s where the whole stage had roughly 9 CPU-minutes. The bug is present at least since the metric was introduced (reproduced on the published `datafusion-physical-plan` 54.1.0 sources and on current main). The newer hash-table implementations already do this correctly with a scoped timer around the whole accumulator loop (`aggregate_hash_table/common.rs` `push_batch`, `aggregate_hash_table/common_ordered.rs`), so this only affects `grouped_hash_stream.rs`. ## To Reproduce Run any GROUP BY with several aggregate functions and compare `aggregation_time` against wall time, e.g. `SELECT k, sum(a), sum(b), sum(c), sum(d), min(a), max(a), count(a), avg(a) FROM t GROUP BY k` over enough rows to make aggregation measurable: `aggregation_time` reads several times the elapsed time of the operator. With a single aggregate function the metric is accurate. ## Expected behavior `aggregation_time` measures the time spent calling accumulators once: `add_elapsed` should run once per interned batch, after the accumulator loop. ## Additional context PR with the one-line move follows. -- 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]
