Add per-aggregate accumulator phase metrics - #24423
Draft
kosiew wants to merge 6 commits into
Draft
Conversation
…ables - Introduce per-agg timers for update/merge/state/evaluate phases. - Use hash and ordered tables for migrated data. - Expose only phase-valid metrics. - Add timing for partial-skip state conversion. - Include tests for partial and final metric labels.
- Central accumulator_phases(mode) helper. - Removed duplicated mode→phase matches. - Added all-mode unit test.
…le modes, cover partitioned variants, and add regression test for forced‑spill ordered‑single
- Adds positive-duration assertions for partial update/state - Adds positive-duration assertions for final merge/evaluate - Retains existing exact label/name checks - References ordered spill/replay coverage added in prior commit
- Centralize hash/ordered metric setup via a private AggregateTableMetrics factory. - Preserve Arc sharing in `from_hash_table`. - Add a table‑driven phase matrix test that retains all 6 modes.
…inal aggregates - Ordered partial now asserts that update.state is positive in update. - Ordered final now asserts that merge is positive in merge evaluation. - Shared test helper introduced to reduce duplication.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ 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 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
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?
update,merge,state, andevaluatephases.aggregation_time, andemitting_timemetrics.GroupsAccumulatorAPI.The new metrics use names such as
agg_expr_0_update_time,agg_expr_0_merge_time,agg_expr_0_state_time, andagg_expr_0_evaluate_time, with the corresponding aggregate expression attached as theaggregatelabel.Are these changes tested?
Yes.
The tests in this PR:
accumulator_phases_match_aggregate_modeto verify the accumulator phases configured for all six aggregate modes.SUM(a)andSUM(b)labels forarguments_time,update_time, andstate_time, and verify positive update/state timings.merge_timeandevaluate_timeforSUM(b)and verify that those timings are positive.datafusion.execution.enable_migration_aggregatein the relevant metrics tests.assert_accumulator_phase_timesto verify positiveupdate/statetimings for partial aggregation andmerge/evaluatetimings for final aggregation.update,state,merge, andevaluatetimings.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.