Refine and document AggregateExec metrics by kosiew · Pull Request #24757 · apache/datafusion

kosiew · 2026-08-28T14:04:06Z

Which issue does this PR close?

Part of Add aggregate specific metrics #23570

Rationale for this change

Aggregate-specific metrics are a user-visible observability surface in EXPLAIN ANALYZE, but their phase boundaries and relationship to individual aggregate expressions need to be clear and consistent.

In particular, users should be able to understand what each aggregate timer measures, map indexed metrics such as agg_expr_0_arguments_time back to the corresponding expression in the operator's aggr=[...] list, and understand which metrics are available on different grouped aggregation paths without reading the implementation.

This PR also tightens the timing boundaries of the grouped aggregate metrics so that the documented metrics correspond more directly to the work they describe.

What changes are included in this PR?

Documents AggregateExec operator-level and per-aggregate metrics, including their phase boundaries, indexed naming convention, aggregate expression labels, visibility, and path-specific coverage.
Groups aggregate timing into explicit phases for:
- group-key preparation,
- aggregate argument and filter evaluation,
- accumulator update / merge operations, and
- output emission.
Ensures accumulator aggregation_time measures a single interval containing the relevant accumulator operations rather than accumulating overlapping/prefix timing.
Includes group interning and ordering setup in time_calculating_group_ids.
Includes collectively evaluated aggregate filters in aggregate_arguments_time.
Adds topk_maintenance_time for Grouped TopK priority-map maintenance and avoids registering aggregation_time for that accumulator-free path.
Times grouped output materialization consistently with emitting_time.
Adds EXPLAIN ANALYZE coverage demonstrating how indexed per-aggregate metrics map to multiple aggregate expressions and how expression labels appear in verbose output.

Are these changes tested?

Yes.

The patch adds and updates tests that verify:

EXPLAIN ANALYZE reports agg_expr_0_arguments_time, agg_expr_1_arguments_time, and agg_expr_2_arguments_time for a query containing SUM(c5), SUM(c6), and COUNT(c7).
EXPLAIN ANALYZE VERBOSE associates those indexed metrics with the corresponding rendered aggregate expressions.
Grouped TopK reports topk_maintenance_time and time_calculating_group_ids while not registering aggregation_time.
Regular aggregate paths do not report topk_maintenance_time.
Aggregate execution without grouped emission does not report emitting_time.
GroupByMetrics::new_topk does not register aggregation_time.

Are there any user-facing changes?

Yes. Aggregate metrics shown through EXPLAIN ANALYZE are clarified and their timing boundaries are made more consistent.

The metrics documentation now describes the AggregateExec operator-level metrics and per-expression timer phases, explains how indexed metrics map to aggregate expressions, and documents path-specific differences such as Grouped TopK and non-grouped aggregation.

These are observability and documentation changes; this PR does not introduce a public API change.

LLM-generated code disclosure

This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.

… mode scope, dev visibility, EXPLAIN example. - Added comprehensive coverage of AggregateExec metrics, including new counters, gauges, and timing measurements. - Documented the distinct metrics **phases** (e.g., parse, optimize, execute) and their significance in performance analysis. - Introduced the **indexes/labels** schema used to tag metrics for efficient querying and grouping. - Clarified the **mode scope** (global vs. session) and how it influences metric collection and visibility. - Expanded the **dev visibility** section with guidelines for developers to monitor and interpret these metrics. - Included a practical **EXPLAIN** example that demonstrates how the new metrics appear in query output.

…hase, indexing, labels, and FILTER handling

- Replace terse description with structured table of metrics - Add details on per‑aggregate timers and phase availability - Clarify which metrics are reported for different aggregation modes and implementations - Update description of time boundaries between `time_calculating_group_ids` and `aggregation_time`

…lementations

- Correct group-ID metric wording/gates. - Add peak_mem_used. - Caveat legacy aggregation_time inflation. - Clarify partial-stage timer example. - Explain Dev vs Summary + config key.

- Refined the TopK timer scope description to clarify precise measurement semantics, removing any ambiguity about timing granularity. - Added explicit documentation for the legacy skip path, detailing its conversion to state via the `convert_to_state` function and providing clear usage examples.

- Correct number of untimed group-key work paths from two to three - Specify that grouping-expression evaluation is not timed for Grouped TopK - Clarify that per-aggregate timers carry an `aggregate` label, visible only in EXPLAIN ANALYZE VERBOSE

…regate timers - Updated `docs/source/user-guide/metrics.md`: - Added explicit description of the scope for TopK untimed group‑expression, clarifying when and how the expression applies. - Revised the label scope section to specify that labels are limited to per‑aggregate timers only.

…d prefix-sum aggregation - Unified grouped timer boundaries. - Fixed legacy aggregation prefix-sum. - Added TopK maintenance metric; omits accumulator metric. - Covered empty grouping-set timer bypasses. - Updated metrics documentation. - Added TopK metric regression assertions.

- Updated the `metrics.md` documentation to clearly distinguish between normal-output and memory-pressure states. - Added explicit timing descriptions for each state, explaining when each occurs and how long it typically lasts. - Refined wording throughout the section to improve readability and remove ambiguity. - Included illustrative examples that contrast the timing behavior of normal-output and memory-pressure states. - Adjusted any related references to ensure consistent terminology across the user guide.

…nd state materialization - Central GroupByMetrics scoped APIs; timer fields private. - TopK metric owned centrally; removed `aggregation_time`. - Migrated aggregate paths to new structure. - State materialization now included in `emitting_time`. - Updated documentation. - Added test for no‑accumulator metric registration.

codecov-commenter · 2026-08-28T14:34:10Z

Codecov Report

❌ Patch coverage is 73.60179% with 118 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.72%. Comparing base (6ab4ce6) to head (9a6efe1).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...hysical-plan/src/aggregates/grouped_hash_stream.rs	50.60%	66 Missing and 16 partials ⚠️
...plan/src/aggregates/aggregate_hash_table/common.rs	82.47%	2 Missing and 15 partials ⚠️
.../aggregates/aggregate_hash_table/common_ordered.rs	80.00%	4 Missing and 10 partials ⚠️
...hysical-plan/src/aggregates/grouped_topk_stream.rs	90.19%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24757      +/-   ##
==========================================
- Coverage   81.72%   81.72%   -0.01%     
==========================================
  Files        1127     1127              
  Lines      416237   416361     +124     
  Branches   416237   416361     +124     
==========================================
+ Hits       340156   340253      +97     
- Misses      56092    56103      +11     
- Partials    19989    20005      +16

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Adjusted the documentation label to accurately reflect the updated terminology. - Changed the function call from `sum(t.a)` to `SUM(a)` to align with the new naming convention. - Ensured consistency between the documentation and the implementation. - Verified that the changes resolve any labeling ambiguities in the documentation. - Updated any related references to use the new label and function name.

…TopK - Added caching of the `is_stream_merging` flag per batch to avoid recomputation on every operation. - Added caching of the `is_group_by_only` flag for each TopK internal call, reducing redundant checks. - Updated the relevant internal methods to store and reuse these cached values, improving performance. - Ensured cache invalidation is synchronized with batch and TopK lifecycle to maintain correctness.

…cked grouped paths, clarify non‑grouped timing and DISTINCT TopK bypass, fix rendered‑label example, and add related tests - Docs scope emitting_time to accumulator‑backed grouped paths. - Docs explain non‑grouped timing + limited DISTINCT TopK bypass. - Corrected rendered‑label example: `sum(t.a)`. - Tests: non‑grouped lacks emitting_time; limited DISTINCT lacks TopK metric. - Added normal/verbose `EXPLAIN` mapping test for 3 aggregates.

…roup values and accumulator results, removing claim of full output‑batch production - Updated the description of `emitting_time` to indicate it materializes group values and accumulator results. - Clarified that the process does not guarantee full output‑batch production. - Adjusted wording to avoid misleading implications about batch completeness.

jayzhan211

Thanks @kosiew ! This LGTM overall — let's get the merge conflict resolved and then it's good to go.

- Removed redundant semicolon after `let hash: Hash256 = ...;` - Eliminated unnecessary semicolon following `let mut stream = ...;` - Cleaned up extra semicolon at the end of a block within `grouped_hash_stream.rs`

**Details:** - Updated the documentation to explicitly require GROUP BY columns to be unordered. - Added a clear note in the SELECT statement documentation about the unordered GROUP BY requirement. - Included examples showing correct unordered GROUP BY usage and highlighting invalid ordered usage. - Adjusted inline docstrings and comment blocks throughout the codebase to reflect the new requirement. - Verified that all documentation builds and linting checks pass, confirming the changes are consistent and error‑free.

… for explain_analyze - Updated `docs/source/user-guide/metrics.md` with the new excerpt describing the metrics feature. - Added a regression assertion in `datafusion/core/tests/sql/explain_analyze.rs` to ensure the `EXPLAIN ANALYZE` behavior remains correct.

kosiew · 2026-09-08T03:59:48Z

🚀
@jayzhan211
Thank you for your review.

kosiew added 11 commits August 28, 2026 20:40

docs: improve documentation for aggregate timers: clarify arguments p…

6efb6cc

…hase, indexing, labels, and FILTER handling

docs(metrics): clarify timing coverage across grouped aggregation imp…

d90dc87

…lementations

docs(metrics): changes for metrics.md

4efb1d4

- Correct group-ID metric wording/gates. - Add peak_mem_used. - Caveat legacy aggregation_time inflation. - Clarify partial-stage timer example. - Explain Dev vs Summary + config key.

github-actions Bot added documentation Improvements or additions to documentation physical-plan Changes to the physical-plan crate labels Aug 28, 2026

kosiew added 4 commits August 28, 2026 22:57

github-actions Bot added the core Core DataFusion crate label Aug 29, 2026

kosiew marked this pull request as ready for review August 29, 2026 09:01

kosiew requested a review from rluvaton August 29, 2026 09:03

jayzhan211 approved these changes Sep 6, 2026

View reviewed changes

Merge branch 'main-24409' into test1

06684f4

kosiew force-pushed the aggmetrics-05-23570 branch from b6d3612 to 06684f4 Compare September 7, 2026 10:49

kosiew added 4 commits September 7, 2026 20:07

fix(grouped_hash_stream): remove redundant semicolons

cb7b61c

- Removed redundant semicolon after `let hash: Hash256 = ...;` - Eliminated unnecessary semicolon following `let mut stream = ...;` - Cleaned up extra semicolon at the end of a block within `grouped_hash_stream.rs`

Merge branch 'main' into aggmetrics-05-23570

3aeacc2

kosiew added this pull request to the merge queue Sep 8, 2026

Merged via the queue into apache:main with commit 8a92281 Sep 8, 2026
44 of 45 checks passed

kosiew deleted the aggmetrics-05-23570 branch September 8, 2026 04:10

kosiew mentioned this pull request Sep 8, 2026

Make ordered aggregate spill test deterministic #25047

Closed

5 tasks

nuno-faria added the v56.0.0 label Oct 3, 2026

Navigation Menu

Repository navigation

Refine and document AggregateExec metrics - #24757

Refine and document AggregateExec metrics#24757
kosiew merged 20 commits into
apache:mainfrom
kosiew:aggmetrics-05-23570

kosiew commented Aug 28, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Aug 28, 2026 •

edited

Loading

Uh oh!

jayzhan211 left a comment

Uh oh!

kosiew commented Sep 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kosiew commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

LLM-generated code disclosure

Uh oh!

codecov-commenter commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

kosiew commented Sep 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kosiew commented Aug 28, 2026 •

edited

Loading

codecov-commenter commented Aug 28, 2026 •

edited

Loading