You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kosiew (via GitHub)" <gi...@apache.org> on 2026/08/17 06:09:42 UTC
[PR] Add per-aggregate accumulator phase metrics [datafusion]
kosiew opened a new pull request, #24423:
URL: https://github.com/apache/datafusion/pull/24423
## Which issue does this PR close?
* Part of #23570
## Rationale for this change
Migrated grouped hash aggregation currently exposes timing for aggregate argument evaluation and shared aggregation/emission work, but does not show how much time each aggregate expression spends performing accumulator operations.
This makes it difficult to identify aggregates where the accumulator itself is the source of execution cost. This PR adds per-aggregate-expression timing for the update, merge, state, and evaluate phases used by migrated hash aggregation.
## What changes are included in this PR?
* Adds per-aggregate accumulator timing metrics for `update`, `merge`, `state`, and `evaluate` phases.
* Creates the applicable phase metrics based on the aggregate mode.
* Instruments accumulator operations in both regular and ordered migrated aggregate tables.
* Covers partial, partial-reduce, final, and single aggregation paths, including state emission and spill/replay paths.
* Reuses aggregate-expression labels so metrics from different aggregate expressions can be distinguished.
* Keeps the existing group-by, aggregate argument, `aggregation_time`, and `emitting_time` metrics.
* Does not change the public `GroupsAccumulator` API.
The new metrics use names such as `agg_expr_0_update_time`, `agg_expr_0_merge_time`, `agg_expr_0_state_time`, and `agg_expr_0_evaluate_time`, with the corresponding aggregate expression attached as the `aggregate` label.
## Are these changes tested?
Yes.
The tests in this PR:
* Add `accumulator_phases_match_aggregate_mode` to verify the accumulator phases configured for all six aggregate modes.
* Extend the aggregate metrics tests to verify separate `SUM(a)` and `SUM(b)` labels for `arguments_time`, `update_time`, and `state_time`, and verify positive update/state timings.
* Extend the final aggregate metrics test to verify `merge_time` and `evaluate_time` for `SUM(b)` and verify that those timings are positive.
* Explicitly enable `datafusion.execution.enable_migration_aggregate` in the relevant metrics tests.
* Extend migrated aggregate execution tests with `assert_accumulator_phase_times` to verify positive `update`/`state` timings for partial aggregation and `merge`/`evaluate` timings for final aggregation.
* Update the spilling single-aggregate test to use a migrated spill context and verify positive `update`, `state`, `merge`, and `evaluate` timings.
## Are there any user-facing changes?
Yes, for users inspecting execution metrics. Migrated hash aggregation now exposes per-aggregate-expression accumulator phase timing metrics, making it possible to distinguish accumulator work from aggregate argument evaluation and shared aggregation/emission work.
There are no changes to aggregate query semantics or public accumulator APIs.
## LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add per-aggregate accumulator phase metrics [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24423:
URL: https://github.com/apache/datafusion/pull/24423#issuecomment-5337998344
@rluvaton
> What about convert_to_states?
[Added a dedicated per-aggregate `convert_to_state` timer for that path and migration-enabled partial-skip coverage.
](https://github.com/apache/datafusion/pull/24423/commits/7f1af777ea618292b3794d0a6060a9031a31db29)
> ..useful to allow for aggregate expressions themself to provide metrics, like array_agg with distinct, there are multiple phases there
For aggregate-internal subphases, I’ll keep this PR at the `GroupsAccumulator` call boundary. It already attributes the complete update/merge/state/evaluate operation to each aggregate expression, including `array_agg(DISTINCT ...)`. Letting an aggregate expose its own submetrics needs a separate metrics API plus naming/ownership contract; it would expand the accumulator trait/API beyond this PR’s stated scope. I’ll work on that as follow-up work.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add per-aggregate accumulator phase metrics [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #24423:
URL: https://github.com/apache/datafusion/pull/24423
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add per-aggregate accumulator phase metrics [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24423:
URL: https://github.com/apache/datafusion/pull/24423#issuecomment-5312666883
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24423?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `95.74468%` with `14 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.24%. Comparing base ([`9a83691`](https://app.codecov.io/gh/apache/datafusion/commit/9a83691aadaadbf72bf4c8b7d87bd28af9dc68dd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`84120c2`](https://app.codecov.io/gh/apache/datafusion/commit/84120c2de92ebb4f4d07c5815401e2b1dcf2e0c4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24423?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...c/aggregates/aggregate\_hash\_table/partial\_table.rs](https://app.codecov.io/gh/apache/datafusion/pull/24423?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Faggregate_hash_table%2Fpartial_table.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL2FnZ3JlZ2F0ZV9oYXNoX3RhYmxlL3BhcnRpYWxfdGFibGUucnM=) | 73.91% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24423?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../aggregates/aggregate\_hash\_table/common\_ordered.rs](https://app.codecov.io/gh/apache/datafusion/pull/24423?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Faggregate_hash_table%2Fcommon_ordered.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL2FnZ3JlZ2F0ZV9oYXNoX3RhYmxlL2NvbW1vbl9vcmRlcmVkLnJz) | 90.62% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24423?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/aggregates/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/24423?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL21vZC5ycw==) | 90.32% | [0 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24423?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...plan/src/aggregates/aggregate\_hash\_table/common.rs](https://app.codecov.io/gh/apache/datafusion/pull/24423?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Faggregate_hash_table%2Fcommon.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL2FnZ3JlZ2F0ZV9oYXNoX3RhYmxlL2NvbW1vbi5ycw==) | 92.30% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24423?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24423 +/- ##
========================================
Coverage 81.23% 81.24%
========================================
Files 1112 1113 +1
Lines 390627 390894 +267
Branches 390627 390894 +267
========================================
+ Hits 317337 317584 +247
- Misses 54656 54671 +15
- Partials 18634 18639 +5
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24423?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add per-aggregate accumulator phase metrics [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24423:
URL: https://github.com/apache/datafusion/pull/24423#issuecomment-5355947024
@rluvaton
Thanks for the review.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add per-aggregate accumulator phase metrics [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #24423:
URL: https://github.com/apache/datafusion/pull/24423#issuecomment-5325889088
What about `convert_to_states`?
also I think it will be useful to allow for aggregate expressions themself to provide metrics, like array_agg with distinct, there are multiple phases there
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org