Skip to content

refactor: extract aggregate physical planning - #26403

Merged
2010YOUY01 merged 1 commit into
apache:mainfrom
1cbyc:1cbyc/26300-extract-aggregate-planning
Oct 16, 2026
Merged

2010YOUY01 merged 1 commit into
apache:mainfrom
1cbyc:1cbyc/26300-extract-aggregate-planning

Conversation

@1cbyc

@1cbyc 1cbyc commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

DefaultPhysicalPlanner::map_logical_node_to_physical contains the complete aggregate planning implementation inline. Moving this arm behind a focused method makes the top-level logical-plan dispatcher easier to navigate and reduces conflict pressure around planner changes.

What changes are included in this PR?

This PR extracts only the Aggregate arm into a private plan_aggregate method. Schema validation, grouping expression lowering, async expression planning, repartition selection, and partial/final aggregate construction are unchanged.

What is the testing strategy for this PR?

  • cargo test -p datafusion --lib physical_planner::tests
  • cargo clippy -p datafusion --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • git diff --check

The existing physical planner tests cover aggregate schema checks, grouping sets, repartitioning, aliases, and scalar subqueries; no new behavior is introduced.

Are there any user-facing changes?

No. This is an internal refactor with no public API or behavior change.

@github-actions github-actions Bot added the core Core DataFusion crate label Oct 16, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.29630% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.87%. Comparing base (5009f42) to head (23a4cd8).

Files with missing lines Patch % Lines
datafusion/core/src/physical_planner.rs 96.29% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #26403   +/-   ##
=======================================
  Coverage   82.87%   82.87%           
=======================================
  Files        1153     1153           
  Lines      457101   457124   +23     
  Branches   457101   457124   +23     
=======================================
+ Hits       378818   378841   +23     
+ Misses      55268    55267    -1     
- Partials    23015    23016    +1     

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

@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!

I've let AI checked this logic has been moved without change

@2010YOUY01
2010YOUY01 added this pull request to the merge queue Oct 16, 2026
Merged via the queue into apache:main with commit b6f6990 Oct 16, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants