[CALCITE-7597] Support ORDER BY ALL#5010
Conversation
tisyabhatia
force-pushed
the
CALCITE-7597-order-by-all
branch
4 times, most recently
from
0759c6e to
e9491a7
Compare
tisyabhatia
force-pushed
the
CALCITE-7597-order-by-all
branch
2 times, most recently
from
e88f311 to
164eed9
Compare
|
You should avoid amending a commit until the review is over, to make it easier for reviewers to find out what's changed |
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
If there are no other comments I plan to merge this
mihaibudiu
added
the
LGTM-will-merge-soon
|
Good to know, thanks for the advice |
xiedeyantu
left a comment
xiedeyantu
left a comment
There was a problem hiding this comment.
Could you also add a few more test cases demonstrating the correct expansion of ORDER BY ALL?
When ORDER BY ALL appears with no order items, sort by every expression in the SELECT clause, in select-list order. An optional trailing ASC/DESC and NULLS FIRST/LAST applies to all keys. The parser emits an ORDER_BY_ALL marker (wrapped in DESC/NULLS_FIRST/NULLS_LAST when a direction is given); SqlValidatorImpl.validateOrderList expands it to the SELECT items, re-applying the direction to each key. SELECT * is rejected. Includes parser + validator tests and reference docs.
tisyabhatia
force-pushed
the
CALCITE-7597-order-by-all
branch
from
c0e6961 to
1f00496
Compare
mihaibudiu
removed
the
LGTM-will-merge-soon
|
@xiedeyantu I have removed the "merge soon" label, please add it if you approve; feel free to merge as well if you are satified |
xiedeyantu
left a comment
xiedeyantu
left a comment
There was a problem hiding this comment.
@mihaibudiu You don't actually need to remove the LGTM label. The new tests look good to me, and I don't have any further comments. @tisyabhatia I think you can squash your commits.
xiedeyantu
added
the
LGTM-will-merge-soon
|
Maybe we can ask @xuzifu666 to take another look and confirm they're happy with the changes. |
…by-all # Conflicts: # core/src/main/java/org/apache/calcite/sql/fun/SqlInternalOperators.java
|
|
Could you squash these commits into one? |
4 participants
JIRA: https://issues.apache.org/jira/browse/CALCITE-7597