Skip to content

Add per-aggregate argument evaluation metrics for grouped hash aggregation - #24024

Open
kosiew wants to merge 6 commits into
apache:mainfrom
kosiew:aggmetrics-01-23570
Open

Add per-aggregate argument evaluation metrics for grouped hash aggregation#24024
kosiew wants to merge 6 commits into
apache:mainfrom
kosiew:aggmetrics-01-23570

Conversation

@kosiew

@kosiew kosiew commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?


Rationale for this change

Grouped hash aggregation currently exposes a single aggregate_arguments_time metric for all aggregate argument evaluation. This makes it difficult to determine which aggregate expression is responsible for argument evaluation cost when multiple aggregates are present (for example, SUM(a) versus SUM(b)).

This change adds per-aggregate argument evaluation metrics while preserving the existing operator-level metric for compatibility.


What changes are included in this PR?

  • Add an AggregateArgumentMetrics helper that registers one timer per aggregate expression during operator construction.

  • Label per-aggregate metrics using the aggregate expression display text (or alias), allowing otherwise identical aggregate functions on different inputs (for example, SUM(a) and SUM(b)) to be distinguished.

  • Wrap aggregate argument evaluation with the corresponding per-aggregate timer in:

    • grouped hash aggregation stream
    • migrated hash table aggregation paths
    • ordered aggregation table paths
  • Introduce OrderedAggregateTableMetrics to carry both group-by and aggregate argument metrics through ordered aggregation replay/spill paths.

  • Factor out a reusable aggregate_metric_label helper for consistent metric labels.

  • Preserve the existing aggregate_arguments_time metric so whole-phase timing continues to be reported.


Are these changes tested?

Yes.

This PR adds and updates tests including:

  • test_groupby_aggregate_argument_metrics_distinguish_inputs, which verifies that separate metrics are created for SUM(a) and SUM(b) with distinct metric names and aggregate labels.

  • Existing group-by metrics tests updated to use shared aggregate construction helpers.

  • Grouped hash aggregation stream tests updated to verify both:

    • the existing aggregate_arguments_time metric remains present and non-zero, and
    • the new per-aggregate metric (agg_expr_0_arguments_time) is emitted and records time.

Are there any user-facing changes?

Yes.

EXPLAIN ANALYZE and execution plan metrics for the covered grouped hash aggregation paths now include per-aggregate argument evaluation timers (for example, agg_expr_0_arguments_time) with aggregate labels identifying the corresponding aggregate expression, while retaining the existing aggregate_arguments_time metric for compatibility.


LLM-generated code disclosure

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

kosiew added 6 commits July 31, 2026 18:09
…metric, wire grouped hash aggregation, and add regression test for SUM labels

- Add AggregateArgumentMetrics
- Add agg_expr_{idx}_arguments_time{aggregate=...}
- Wire grouped hash:
- legacy GroupedHashAggregateStream
- migrated hash table
- ordered migrated table
- skip-partial conversion path
- Add regression test for SUM(a) vs SUM(b) labels
…ric_label, add AggregateArgumentMetrics::time helper, simplify timer guard boilerplate, narrow test metric filter, and add sum_aggregate and count_aggregate test helpers
…s for ordered tables

- Introduce a private `OrderedAggregateTableMetrics` class that bundles `GroupByMetrics` and `AggregateArgumentMetrics`.
- Update ordered table constructors to accept the combined metrics bundle instead of separate `group_by_metrics` and `partition` parameters.
- Modify spill replay logic to reuse the full metrics bundle from the table, eliminating the need for recomputation.
…g_expr_0_arguments_time are measured and non‑zero; adjust skip‑partial test to use two batches

- Wrap aggregate argument evaluation in `transform_to_states` with existing `aggregate_arguments_time`
- Update the skip‑partial test to use two batches to exercise the skip path
- Add assertions that both `aggregate_arguments_time` and `agg_expr_0_arguments_time` exist and are non‑zero
…d debug_assert! bounds check in AggregateArgumentMetrics::time
@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Jul 31, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.34234% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.86%. Comparing base (f8b9ed8) to head (a43dd63).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ysical-plan/src/aggregates/group_values/metrics.rs 88.54% 1 Missing and 14 partials ⚠️
...hysical-plan/src/aggregates/grouped_hash_stream.rs 94.73% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #24024    +/-   ##
========================================
  Coverage   80.86%   80.86%            
========================================
  Files        1099     1099            
  Lines      374720   374904   +184     
  Branches   374720   374904   +184     
========================================
+ Hits       303015   303183   +168     
- Misses      53614    53623     +9     
- Partials    18091    18098     +7     

☔ 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
kosiew marked this pull request as ready for review July 31, 2026 11:11
@kosiew
kosiew requested a review from rluvaton July 31, 2026 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants