kosiew opened a new pull request, #24523: URL: https://github.com/apache/datafusion/pull/24523
## Which issue does this PR close? * Part of #23570 ## Rationale for this change Aggregate-specific timing metrics should remain consistent across grouped aggregation implementations. Previously, queries using the legacy grouped hash or grouped TopK paths could expose less per-aggregate timing information than queries using the migrated grouped hash implementation, making metrics dependent on which internal aggregation path was selected. This change extends aggregate-specific metrics to those remaining paths for the phases they actually perform. ## What changes are included in this PR? * Adds per-aggregate accumulator timing metrics to `GroupedHashAggregateStream` for update, merge, state, evaluate, and convert-to-state phases. * Reuses the same aggregate labels and accumulator metric helpers used by the migrated grouped aggregation implementation. * Adds per-aggregate argument evaluation timing to `GroupedTopKAggregateStream`. * Documents that grouped TopK does not expose accumulator update, merge, state, or evaluate timing because it maintains MIN/MAX values directly in its priority map. * Extends final-mode metric coverage to exercise both values of `datafusion.execution.enable_migration_aggregate`. ## Are these changes tested? Yes. The patch adds `test_legacy_groupby_aggregate_accumulator_metrics`, which disables `datafusion.execution.enable_migration_aggregate` and verifies aggregate-specific `arguments_time`, `update_time`, and `state_time` metrics for the legacy grouped hash path, including positive update and state timings. `test_groupby_metrics_final_mode` now exercises final-mode metrics with `datafusion.execution.enable_migration_aggregate` set to both `true` and `false`. The patch also adds `test_topk_aggregate_argument_metrics`, which exercises grouped TopK aggregation and verifies that the per-aggregate `agg_expr_0_arguments_time` metric is present, labeled with `MIN(a)`, and non-zero. ## Are there any user-facing changes? Yes. Query execution metrics now expose more consistent aggregate-specific timing information when grouped aggregation uses the legacy grouped hash or grouped TopK implementations. There are no public API changes. ## 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]
