-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
[SPARK-32220][SQL][FOLLOW-UP]SHUFFLE_REPLICATE_NL Hint should not change Non-Cartesian Product join result #29084
Conversation
// Since for join hint [[SHUFFLE_REPLICATE_NL]] when having equi join conditions, | ||
// we still choose Cartesian Product Join, but in ExtractEquiJoinKeys, it will filter | ||
// out equi condition. Instead of using the condition extracted by ExtractEquiJoinKeys, | ||
// we should use the original join condition "j.condition". |
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.
how about:
`CartesianProductExec` can't implicitly evaluate the equal join condition, here we should
pass the original condition which includes both equal and non-equal conditions.
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.
how about:
`CartesianProductExec` can't implicitly evaluate the equal join condition, here we should pass the original condition which includes both equal and non-equal conditions.
Updated
Test build #125754 has finished for PR 29084 at commit
|
retest this please |
Test build #125757 has finished for PR 29084 at commit
|
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.
+1, LGTM. Thank you, @AngersZhuuuu and all.
Merged to master.
Could you make a backporting PR to branch-3.0, @AngersZhuuuu ? |
Test build #125771 has finished for PR 29084 at commit
|
Sure, |
What changes were proposed in this pull request?
follow comment #29035 (comment)
Explain for pr
Why are the changes needed?
add comment
Does this PR introduce any user-facing change?
No
How was this patch tested?
Not need