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-41637][SQL] ORDER BY ALL #39144
Conversation
|
||
override def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUpWithPruning( | ||
_.containsAllPatterns(UNRESOLVED_ATTRIBUTE, SORT), ruleId) { | ||
// This only makes sense if the children is resolved. |
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: child, not children
cc @cloud-fan this one too. seems much easier than group by all. |
@@ -0,0 +1,44 @@ | |||
create temporary view data as select * from values |
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: we can have a test case combining group by all
and order by all
. It should work, just for covering a testing scenario.
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.
isn't it better to test different features in different golden files?
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.
Oh you mean test both feature in one query? That's a good idea, we can add later.
thanks, merging to master! |
What changes were proposed in this pull request?
This patch adds ORDER BY ALL support to SQL. ORDER BY ALL is a syntactic sugar to sort the output by all the fields, from left to right. It also allows specifying asc/desc as well as null ordering.
Why are the changes needed?
It's a good convenience sugar added initially by DuckDB to avoid repeating the fields.
Does this PR introduce any user-facing change?
Yes. See above.
How was this patch tested?
Added SQL tests.