-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Reuse CometBroadcastExchangeExec with Spark ReuseExchangeAndSubquery rule #441
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #441 +/- ##
=========================================
Coverage 34.17% 34.17%
Complexity 850 850
=========================================
Files 116 116
Lines 38547 38552 +5
Branches 8523 8524 +1
=========================================
+ Hits 13172 13176 +4
- Misses 22609 22610 +1
Partials 2766 2766 ☔ View full report in Codecov by Sentry. |
c41c097
to
67acabc
Compare
67acabc
to
4e174d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
case plan if plan.children.exists(_.isInstanceOf[BroadcastExchangeExec]) => | ||
val newChildren = plan.children.map { | ||
case b: BroadcastExchangeExec | ||
if isCometNative(b.child) && | ||
isCometOperatorEnabled(conf, "broadcastExchangeExec") => | ||
isCometOperatorEnabled(conf, "broadcastExchangeExec") && | ||
isSpark34Plus => // Spark 3.4+ only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If BroadcastExchangeExec
is only for 3.4+, is isSpark34Plus
extra?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CometBroadcastExchangeExec
is only for 3.4+ not BroadcastExchangeExec
.
Merged. Thanks @kazuyukitanimura @sunchao |
…uery rule (apache#441) (cherry picked from commit ec8da30)
Which issue does this PR close?
Closes #439.
Rationale for this change
What changes are included in this PR?
How are these changes tested?