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: [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]
