jayzhan211 commented on PR #25051:
URL: https://github.com/apache/datafusion/pull/25051#issuecomment-5645782372
**`invoke_per_accumulator` regresses the adapter path ~20% for aggregates
with no submetric**
The chunked rewrite in
`datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs:300-350`
replaces one pass over `groups_with_rows` with a per-64-group `Vec<(usize,
Vec<ArrayRef>)>` plus three passes over it. It exists only so the timer can
wrap a bounded batch of `f` calls, but it runs unconditionally — and
`grouped_update_metric` is `Some` for exactly one aggregate today. Everything
else on the adapter path (`string_agg`, ordered `array_agg`, `nth_value`,
`approx_percentile_cont`, every third-party UDAF) pays the cost and gets no
metric.
Measured on this branch vs. `b239d041b5`:
`GroupsAccumulatorAdapter::update_batch`, 8192-row batch, 8192 groups (1
row/group), `MaxAccumulator`, release, 300 iterations:
| | µs/iter |
|---|---|
| main (`b239d041b5`) | 383.1 / 386.7 / 387.4 |
| this PR | 462.3 / 472.1 / 478.5 |
| this PR + fast path below | 399.1 / 411.7 / 412.4 |
`convert_to_state` is unaffected (~665 µs/iter both ways), so only
`invoke_per_accumulator` needs the guard:
```diff
let result: Result<()> = (|| {
+ if grouped_update_metric.is_none() {
+ // Untimed fast path: identical to the pre-metrics loop.
+ for (&group_idx, offsets) in
+ groups_with_rows.iter().zip(offsets.windows(2))
+ {
+ let state = &mut self.states[group_idx];
+ sizes_pre += state.size();
+ let values_to_accumulate = slice_and_maybe_filter(
+ &values,
+ opt_filter.as_ref().map(|f| f.as_boolean()),
+ offsets,
+ )?;
+ f(state.accumulator.as_mut(), &values_to_accumulate)?;
+ let state = &mut self.states[group_idx];
+ state.indices.clear();
+ sizes_post += state.size();
+ }
+ return Ok(());
+ }
// Keep preparation bounded to avoid retaining one filtered
array per
// group. Time only accumulator invocation: slicing and
filtering
// are adapter work, not aggregate-owned subphase work.
```
That's the exact patch I benchmarked. Note it also makes
`time_grouped_update` (currently `true` at both call sites) genuinely dead —
worth removing in the same pass.
--
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]
