Skip to content

feat: add destination-aware native shuffle execution (3/n) - #5481

Merged
sunchao merged 1 commit into
apache:mainfrom
pingzh:pingzh-rss-destination-aware-native-shuffle
Aug 27, 2026
Merged

feat: add destination-aware native shuffle execution (3/n)#5481
sunchao merged 1 commit into
apache:mainfrom
pingzh:pingzh-rss-destination-aware-native-shuffle

Conversation

@pingzh

@pingzh pingzh commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #5352. This is the third foundational PR and does not close the issue.

Previous PRs:

Rationale for this change

Previous PRs introduced the RSS partition writer and destination-aware shuffle plans, but native shuffle execution still supports only local files. This PR enables the existing shuffle execution path to write to either local files or a remote shuffle service.

What changes are included in this PR?

  • Introduce ShuffleWriterDestination with local and RSS destinations.
  • Add a destination-aware ShuffleWriterExec constructor while preserving the existing local constructor.
  • Reuse existing partitioners for both local and remote shuffle output.
  • Preserve typed callback errors and enforce RSS frame-size limits.
  • Update spill logging and metrics documentation for remote destinations.
  • Add end-to-end coverage for compression codecs, partitioning strategies, empty schemas, remote spilling, callback failures, and local compatibility.

Binding RSS destinations to task-owned JVM callbacks will be addressed in a subsequent PR.

How are these changes tested?

  • Added 10 end-to-end RSS execution tests.
  • Passed 98 Rust tests across shuffle, JNI bridge, protobuf, and planner suites.
  • Passed all 31 tests in CometNativeShuffleSuite.
  • Passed workspace-wide Clippy, Rust formatting checks, and the full Maven reactor build.

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

Reviewed commit 8281c3490c9cc007e846d02f3bc6ef500bf7ba16 with five independent reviewers covering execution lifecycle, partitioning/spills, callbacks/framing, local compatibility, and edge cases. No outstanding verified P1/P2 findings.

Local validation passed: 60 shuffle unit tests (including all 10 added tests), 24 local/RSS output comparisons, 3 partial callback failure checks, and formatting/diff checks. Actual JVM/RSS integration was not exercised; callback binding is deferred.

@sunchao
sunchao merged commit 4897161 into apache:main Aug 27, 2026
138 of 141 checks passed
@sunchao

sunchao commented Aug 27, 2026

Copy link
Copy Markdown
Member

Merged, thanks @pingzh !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants