Skip to content

feat: add early emit count metric to partial aggregates - #24996

Open
yashrb24 wants to merge 4 commits into
apache:mainfrom
yashrb24:feat/aggregate-early-emit-metric
Open

feat: add early emit count metric to partial aggregates#24996
yashrb24 wants to merge 4 commits into
apache:mainfrom
yashrb24:feat/aggregate-early-emit-metric

Conversation

@yashrb24

@yashrb24 yashrb24 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Why make this change?

When DataFusion is working on a partial GROUP BY and memory fills up, it sends out the work completed so far and starts collecting again. Today, EXPLAIN ANALYZE does not show that this happened. This makes it hard to tell why the partial GROUP BY produced more rows than expected.

What changed?

This PR adds an early_emit_count number to EXPLAIN ANALYZE. It goes up once each time we send out partial results because memory is full.

If one set of partial results is sent in several smaller pieces, the number still goes up only once.

This change does not affect query results and does not add new logs.

How was it tested?

Tests cover each way DataFusion runs a partial GROUP BY. They check that:

  • the number goes up once for each memory-related send
  • splitting one send into smaller pieces does not increase it more than once
  • other ways of sending partial results do not increase it

Are there any user-facing changes?

EXPLAIN ANALYZE can now show early_emit_count for partial GROUP BY steps.

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Sep 6, 2026

@2010YOUY01 2010YOUY01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this makes sense to me. It should be good to go after reverting some changes.

One optional suggestion is: we can test partial mode aggregate in sqllogictest for this metric. (PartialReduce mode is not applicable for SQL interface)

reduction_factor: Option<metrics::RatioMetrics>,

/// Number of times accumulated states were emitted due to memory pressure.
early_emit_count: Option<metrics::Count>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's revert the changes in this file. It's deprecated now, and will no longer get maintained
#24961

reservation: MemoryReservation,
baseline_metrics: BaselineMetrics,
reduction_factor: metrics::RatioMetrics,
early_emit_count: metrics::Count,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In OrderedPartialAggregateStream, intermediate states are doing early emit for a different reason, I think reusing the same metric name can cause confusion, and it also seems less useful. Overall I recommend to remove it here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! makes sense, will fix this

@codecov-commenter

codecov-commenter commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.70%. Comparing base (262936e) to head (a5e32f7).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...fusion/physical-plan/src/aggregates/hash_stream.rs 90.90% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24996      +/-   ##
==========================================
+ Coverage   81.67%   81.70%   +0.02%     
==========================================
  Files        1126     1127       +1     
  Lines      414842   415699     +857     
  Branches   414842   415699     +857     
==========================================
+ Hits       338841   339643     +802     
- Misses      56070    56110      +40     
- Partials    19931    19946      +15     

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

@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Sep 7, 2026
@yashrb24
yashrb24 force-pushed the feat/aggregate-early-emit-metric branch from 6bee1d6 to a5e32f7 Compare September 7, 2026 07:04
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 sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show when partial aggregation flushes because of memory pressure

3 participants