-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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-30433][SQL] Make conflict attributes resolution more scalable in ResolveReferences #27105
Conversation
cc @cloud-fan |
} | ||
|
||
/* | ||
* Note that it's possible for conflictPlans to be empty while it implies that there |
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.
`conflictPlans`
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.
`conflictPlans` can be empty which implies that ...
// transformDown so that we can replace all the old Relations in one turn due to | ||
// the reason that conflictPlans are also collected in pre-order. | ||
right transformDown { | ||
case r if conflictPlanMap.contains(r) => conflictPlanMap(r) |
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.
nit: case r => conflictPlanMap.getOrElse(r, r)
Test build #116153 has finished for PR 27105 at commit
|
Test build #116168 has finished for PR 27105 at commit
|
thanks, merging to master! |
What changes were proposed in this pull request?
This PR tries to make conflict attributes resolution in
ResolveReferences
more scalable by doing resolution in batch way.Why are the changes needed?
Currently,
ResolveReferences
rule only resolves conflict attributes of one single conflict plan pair in one iteration, which can be inefficient when there're many conflicts.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Covered by existed tests.