-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-33832][SQL] Force skew join code simplification and improvement #34080
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
Conversation
|
cc @ulysses-you |
| assert(requiredChildOrderings.length == originalChildren.length) | ||
| // Ensure that the operator's children satisfy their output distribution requirements. | ||
| var newChildren = originalChildren.zip(requiredChildDistributions).map { | ||
| var children = originalChildren.zip(requiredChildDistributions).map { |
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.
I feel the original name children is better, as it's a var and we keep updating it.
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #143559 has finished for PR 34080 at commit
|
ulysses-you
left a comment
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
yaooqinn
left a comment
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
|
thanks for review, merging to master! |
What changes were proposed in this pull request?
This is a followup of #32816 to simplify and improve the code:
SkewJoinChildWrapperto wrap the skew join children, so thatEnsureRequirementsrule will skip them and save timeSkewJoinAwareCostand keep usingSimpleCost. We can putnumSkewsin the first 32 bits.Why are the changes needed?
code simplification and improvement
Does this PR introduce any user-facing change?
No
How was this patch tested?
existing tests