Skip to content

[CALCITE-7597] Support ORDER BY ALL#5010

Open
tisyabhatia wants to merge 1 commit into
apache:mainfrom
tisyabhatia:CALCITE-7597-order-by-all
Open

[CALCITE-7597] Support ORDER BY ALL#5010
tisyabhatia wants to merge 1 commit into
apache:mainfrom
tisyabhatia:CALCITE-7597-order-by-all

Conversation

@tisyabhatia

@tisyabhatia tisyabhatia commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

@tisyabhatia tisyabhatia force-pushed the CALCITE-7597-order-by-all branch 4 times, most recently from 0759c6e to e9491a7 Compare June 10, 2026 19:12
mihaibudiu
mihaibudiu requested changes Jun 10, 2026
View reviewed changes
Comment thread core/src/main/java/org/apache/calcite/sql/fun/SqlInternalOperators.java Outdated
Comment thread site/_docs/reference.md Outdated
Comment thread core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
@mihaibudiu mihaibudiu mentioned this pull request Jun 10, 2026
Merged
@tisyabhatia tisyabhatia force-pushed the CALCITE-7597-order-by-all branch 2 times, most recently from e88f311 to 164eed9 Compare June 11, 2026 18:38
@tisyabhatia tisyabhatia requested a review from mihaibudiu June 11, 2026 18:40
@mihaibudiu

mihaibudiu commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

You should avoid amending a commit until the review is over, to make it easier for reviewers to find out what's changed

mihaibudiu
mihaibudiu approved these changes Jun 11, 2026
View reviewed changes

@mihaibudiu mihaibudiu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are no other comments I plan to merge this

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jun 11, 2026
@tisyabhatia

tisyabhatia commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Good to know, thanks for the advice

xiedeyantu
xiedeyantu reviewed Jun 12, 2026
View reviewed changes

@xiedeyantu xiedeyantu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a few more test cases demonstrating the correct expansion of ORDER BY ALL?

Comment thread testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java
xuzifu666
xuzifu666 reviewed Jun 12, 2026
View reviewed changes
Comment thread core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
@tisyabhatia tisyabhatia force-pushed the CALCITE-7597-order-by-all branch from c0e6961 to 1f00496 Compare June 15, 2026 18:11
@mihaibudiu mihaibudiu removed the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jun 15, 2026
@mihaibudiu

mihaibudiu commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

@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
xiedeyantu approved these changes Jun 16, 2026
View reviewed changes

@xiedeyantu xiedeyantu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 xiedeyantu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jun 16, 2026
@xiedeyantu

xiedeyantu commented Jun 16, 2026

Copy link
Copy Markdown
Member

Maybe we can ask @xuzifu666 to take another look and confirm they're happy with the changes.

@xiedeyantu

xiedeyantu commented Jun 20, 2026

Copy link
Copy Markdown
Member

Could you squash these commits into one?

@tisyabhatia tisyabhatia force-pushed the CALCITE-7597-order-by-all branch from 049d3ae to 16f2f1c Compare June 22, 2026 15:03
@tisyabhatia

tisyabhatia commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

commits squashed @xiedeyantu

@sonarqubecloud

sonarqubecloud Bot commented Jun 22, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xuzifu666 xuzifu666 xuzifu666 approved these changes

@mihaibudiu mihaibudiu mihaibudiu approved these changes

@xiedeyantu xiedeyantu xiedeyantu approved these changes

Assignees

No one assigned

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tisyabhatia @mihaibudiu @xiedeyantu @xuzifu666