-
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-12705] [SQL] push missing attributes for Sort #11153
Conversation
cc @marmbrus |
Test build #51040 has finished for PR 11153 at commit
|
Test build #2533 has finished for PR 11153 at commit
|
cc @gatorsmile |
Sorry, my previous fix does not cover all the scenarios. Will read your fix tomorrow. Thanks! |
Test build #51085 has finished for PR 11153 at commit
|
case other => (Seq.empty[SortOrder], Seq.empty[Attribute]) | ||
val newAggregateExpressions = a.aggregateExpressions ++ missingAttrs | ||
a.copy(aggregateExpressions = newAggregateExpressions) | ||
case u: UnaryNode => |
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.
It sounds like we want to cover all the UnaryNode
here? This is different from what we discussed. I am fine if you want to add more supports (e.g., crossing the boundary of subquery), but we might need to add more test cases to ensure it does not break anything.
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 needed, I can add more test cases after this is merged, since it could be time-consuming. Please feel free to let me know.
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.
Will exclude Subquery, feel free to add more tests.
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.
Sure, will do.
Test build #51124 has finished for PR 11153 at commit
|
LGTM |
cc @cloud-fan for review too |
LGTM except one comment |
Merging this into master, thanks! |
The current implementation of ResolveSortReferences can only push one missing attributes into it's child, it failed to analyze TPCDS Q98, because of there are two missing attributes in that (one from Window, another from Aggregate).