Skip to content

Refine and document AggregateExec metrics - #24757

Open
kosiew wants to merge 15 commits into
apache:mainfrom
kosiew:aggmetrics-05-23570
Open

Refine and document AggregateExec metrics#24757
kosiew wants to merge 15 commits into
apache:mainfrom
kosiew:aggmetrics-05-23570

Conversation

@kosiew

@kosiew kosiew commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

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.

kosiew added 11 commits August 28, 2026 20:40
… 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.
- 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`
- 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.
@github-actions github-actions Bot added documentation Improvements or additions to documentation physical-plan Changes to the physical-plan crate labels Aug 28, 2026
@codecov-commenter

codecov-commenter commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.18121% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.51%. Comparing base (c56a7ab) to head (608174b).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...c/aggregates/aggregate_hash_table/partial_table.rs 2.22% 44 Missing ⚠️
...hysical-plan/src/aggregates/grouped_hash_stream.rs 81.92% 11 Missing and 19 partials ⚠️
.../aggregates/aggregate_hash_table/common_ordered.rs 80.00% 4 Missing and 10 partials ⚠️
...plan/src/aggregates/aggregate_hash_table/common.rs 82.69% 2 Missing and 7 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.47%   81.51%   +0.03%     
==========================================
  Files        1122     1123       +1     
  Lines      404140   405263    +1123     
  Branches   404140   405263    +1123     
==========================================
+ Hits       329284   330340    +1056     
- Misses      55546    55584      +38     
- Partials    19310    19339      +29     

☔ 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.

kosiew added 4 commits August 28, 2026 22:57
- 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.
@github-actions github-actions Bot added the core Core DataFusion crate label Aug 29, 2026
@kosiew
kosiew marked this pull request as ready for review August 29, 2026 09:01
@kosiew
kosiew requested a review from rluvaton August 29, 2026 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate documentation Improvements or additions to documentation physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants