Skip to content

docs: note that the peak native aggregate memory metric is not reported - #6557

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:docs/agg-peak-memory-not-reported
Oct 3, 2026
Merged

andygrove merged 1 commit into
apache:mainfrom
andygrove:docs/agg-peak-memory-not-reported

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

Related to #5703. This PR does not close it.

Rationale for this change

The user guide lists peak native aggregate memory as a working CometHashAggregate metric, but it never gets a value.

#5423 added the metric. Since the DataFusion 55 upgrade (#5262), DataFusion picks new aggregate streams by default (datafusion.execution.enable_migration_aggregate = true). Those streams register the spill metrics but not peak_mem_used. Only the older GroupedHashAggregateStream does. Comet doesn't change that setting, so the native aggregate never reports the metric. #5262 ignored the two CometAggregateSuite tests that check it, and #5703 tracks re-enabling them.

The three aggregate spill metrics in the same table are not affected.

What changes are included in this PR?

One row in docs/source/user-guide/latest/metrics.md now says the metric isn't currently reported and links #5703. When #5703 is fixed, the row can go back to the plain description.

How are these changes tested?

This is a docs-only change. npx prettier@latest --check passes on the file.

To confirm the cause, I checked the DataFusion 55.1.0 sources that Comet builds against: peak_mem_used is registered only in aggregates/grouped_hash_stream.rs. DataFusion main is the same as of 2026-10-01.

Since the DataFusion 55 upgrade, the aggregate streams DataFusion picks by
default do not register peak_mem_used, so CometHashAggregateExec's
'peak native aggregate memory' metric never gets a value. The user guide
listed it as working. Point readers at apache#5703 until it is fixed.
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Oct 2, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Summary

  • Prior state and problem: The guide described peak native aggregate memory without acknowledging that the default aggregate implementation does not report it.
  • Design approach: Add a warning to the existing metric description and link the open tracking issue, #5703.
  • Correctness / compatibility analysis: Verified Comet’s metric forwarding and DataFusion 55.1.0’s stream selection. Migrated streams omit peak_mem_used, while the legacy stream registers it. Spark sources for supported 3.4, 3.5, 4.0, 4.1 and experimental 4.2 independently report peakMemory. The warning accurately describes Comet’s default behavior.
  • Key design decisions: Keeping the metric row preserves discoverability and connects the limitation to its tracked resolution. No additional abstraction or complexity is introduced.
  • Implementation sketch: One description changes in docs/source/user-guide/latest/metrics.md:85. The full base-relative diff contains no other changes.
  • Behavioral changes worth calling out: Documentation now acknowledges the existing reporting gap. Runtime behavior, spill metrics and performance are unchanged.
  • Suggested improvements: None meeting the requested review bar. No introduced P1/P2 issues found within this review.

Reviewed full SHA f4a75cc9cb0aeadb4ea86e2bcdd4109d7483105a against base 97ea38f838aa705e3c9465e24734d6df0ffc976b. The PR remains non-draft. Read AGENTS.md and used review-comet-pr. No sibling skill applies to this documentation-only diff. The snapshot and live discussion contain no reviews, issue comments, inline comments or review threads.

Exact-head CI: 7 successful checks and 16 skipped jobs, with no failures or running checks. Spark, native and documentation builds were skipped.

Validation: git diff --check and a table-structure/content check passed. Compared relevant upstream Spark sources and inspected pinned DataFusion sources. No Spark/native tests or documentation build were run. Prettier was not rerun because npx is unavailable.

@andygrove
andygrove added this pull request to the merge queue Oct 3, 2026
Merged via the queue into apache:main with commit 03cb6bd Oct 3, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants