Skip to content

chore: Refactor CometExecRule handling of exchanges [WIP]#2848

Closed
andygrove wants to merge 20 commits intoapache:mainfrom
andygrove:refactor-op2proto2
Closed

chore: Refactor CometExecRule handling of exchanges [WIP]#2848
andygrove wants to merge 20 commits intoapache:mainfrom
andygrove:refactor-op2proto2

Conversation

@andygrove
Copy link
Member

@andygrove andygrove commented Dec 4, 2025

Which issue does this PR close?

Follows on from #2844

Rationale for this change

Clean up hacky convoluted code and use the serde framework for exchanges for more consistency.

The size of CometExecRule is reduced from ~900 lines to ~500 lines with this PR.

What changes are included in this PR?

  • Remove CometExecRule.operator2Proto 🎉
  • CometBroadcastExchangeExec and CometShuffleExchangeExec now implement CometSink
  • Move logic for determing whether shuffles are supported into CometShuffleExchangeExec
  • Add requiresNativeChildren to CometOperatorSerde so we no longer need methods like convertToComet and convertToCometIfAllChildrenAreNative. It makes more sense for each operator to know whether it requires native children.

How are these changes tested?

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2025

Codecov Report

❌ Patch coverage is 74.70817% with 65 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.40%. Comparing base (f09f8af) to head (339f744).
⚠️ Report is 737 commits behind head on main.

Files with missing lines Patch % Lines
...t/execution/shuffle/CometShuffleExchangeExec.scala 73.41% 27 Missing and 19 partials ⚠️
...n/scala/org/apache/comet/rules/CometExecRule.scala 74.32% 13 Missing and 6 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##               main    #2848       +/-   ##
=============================================
- Coverage     56.12%   42.40%   -13.73%     
- Complexity      976      983        +7     
=============================================
  Files           119      167       +48     
  Lines         11743    15200     +3457     
  Branches       2251     2512      +261     
=============================================
- Hits           6591     6445      -146     
- Misses         4012     7726     +3714     
+ Partials       1140     1029      -111     

☔ View full report in Codecov by Sentry.
📢 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.

@andygrove andygrove changed the title chore: Refactor CometExecRule handling of exchanges [WIP] chore: Refactor CometExecRule handling of exchanges Dec 4, 2025
@andygrove andygrove marked this pull request as ready for review December 4, 2025 22:04
@andygrove andygrove requested a review from mbutrovich December 4, 2025 22:04
@andygrove andygrove marked this pull request as draft December 5, 2025 19:16
@andygrove andygrove changed the title chore: Refactor CometExecRule handling of exchanges chore: Refactor CometExecRule handling of exchanges [WIP] Dec 5, 2025
@andygrove
Copy link
Member Author

I am going to break this out into some smaller PRs

@andygrove andygrove closed this Dec 5, 2025
@andygrove andygrove deleted the refactor-op2proto2 branch December 12, 2025 16:02
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