Repository navigation
minor: Clean up shuffle transformation code in CometExecRule - #2840
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2840 +/- ##
============================================
+ Coverage 56.12% 59.16% +3.04%
- Complexity 976 1490 +514
============================================
Files 119 167 +48
Lines 11743 15272 +3529
Branches 2251 2523 +272
============================================
+ Hits 6591 9036 +2445
- Misses 4012 4945 +933
- Partials 1140 1291 +151 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| // try native shuffle first, then columnar shuffle, then fall back to Spark | ||
| // if neither are supported | ||
| tryNativeShuffle(s).orElse(tryColumnarShuffle(s)).getOrElse(s) |
There was a problem hiding this comment.
This is much easier to comprehend, IMO
comphead
left a comment
There was a problem hiding this comment.
Thanks @andygrove this is a nice clean up
| def operator2ProtoIfAllChildrenAreNative(op: SparkPlan): Option[Operator] = { | ||
| if (op.children.forall(_.isInstanceOf[CometNativeExec])) { | ||
| operator2Proto(op, op.children.map(_.asInstanceOf[CometNativeExec].nativeOp): _*) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Convert operator to proto and then apply a transformation to wrap the proto in a new plan. | ||
| */ | ||
| def newPlanWithProto(op: SparkPlan, fun: Operator => SparkPlan): SparkPlan = { | ||
| operator2ProtoIfAllChildrenAreNative(op).map(fun).getOrElse(op) | ||
| } |
There was a problem hiding this comment.
I moved these methods out of the transform method so that they can be accessed elsewhere
mbutrovich
left a comment
There was a problem hiding this comment.
I love a negative line count! Thanks @andygrove
| .flatMap(_ => operator2ProtoIfAllChildrenAreNative(s)) | ||
| .map { nativeOp => | ||
| // Switch to use Decimal128 regardless of precision, since Arrow native execution | ||
| // doesn't support Decimal32 and Decimal64 yet. |
There was a problem hiding this comment.
Unrelated: I'm not sure if this is true anymore. I saw a lot of code related to Decimal32 and Decimal64 in DataFusion these days, so we might want to revisit this assumption from the early days of Comet.
|
Thanks for the reviews @comphead @mbutrovich |
Which issue does this PR close?
N/A
Rationale for this change
The shuffle transformation code that I previously implemented is not very easy to follow. This PR refactors it to be more readable and concise.
What changes are included in this PR?
How are these changes tested?