Skip to content
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

[CALCITE-6063] If ARRAY subquery has ORDER BY (without LIMIT), rows are not sorted #3644

Merged
merged 1 commit into from
Jan 27, 2024

Conversation

chucheng92
Copy link
Member

@chucheng92 chucheng92 commented Jan 24, 2024

issue:
https://issues.apache.org/jira/browse/CALCITE-6063

context:
The sql-standard-2003 allows array constrcutor by query:

<array value constructor by query> ::=
   ARRAY <left paren>  <query expression>  [ <order by clause>  ] <right paren> 

and calcite does support it.

however the orderby was removed by the issue https://issues.apache.org/jira/browse/CALCITE-2978.

The solution of this PR is same as #1763 to allow sort in subquery.

Copy link

sonarcloud bot commented Jan 24, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

14 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -5538,7 +5538,7 @@ ImmutableList<RelNode> retrieveCursors() {
case ARRAY_QUERY_CONSTRUCTOR:
call = (SqlCall) expr;
query = Iterables.getOnlyElement(call.getOperandList());
root = convertQueryRecursive(query, false, null);
root = convertQueryRecursive(query, true, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this deserves a comment. I think you are pretending this to be the toplevel query, because this will prevent ORDER BY from being eliminated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. Perhaps rename boolean top to boolean preserveOrderBy (assuming that top doesn't control any other behavior). Then the documentation can be on the convertQueryRecursive method.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, fixed. however I still used top, The current codepath is passed in from top all the way. top is also easy to understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

You force-pushed?!

Copy link
Member Author

Choose a reason for hiding this comment

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

You force-pushed?!

Sorry, I know the rules for review, but I accidentally force-push it ..


| Operator syntax | Description |
|:-----------------------------------|:------------|
| ARRAY (sub-query) | Creates an array from the result of a sub-query. Example: `ARRAY(SELECT empno FROM emp)` |
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add ORDER BY to example, to illustrate that it is possible

Copy link
Member Author

Choose a reason for hiding this comment

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

Added it.

@chucheng92 chucheng92 changed the title [CALCITE-6063] ARRAY subquery with OrderBy loses Sort [CALCITE-6063] If ARRAY subquery has ORDER BY(without LIMIT), rows are not sorted Jan 25, 2024
@chucheng92 chucheng92 force-pushed the CALCITE-6063 branch 2 times, most recently from 8c978fc to 0265ec5 Compare January 25, 2024 06:54
@chucheng92
Copy link
Member Author

chucheng92 commented Jan 25, 2024

I have updated the PR to fix:

  1. Add example with OrderBy
  2. Add test cases for MULTISET & MAP subquery (including parser & operator test)

It should be noted that there are some differences between ARRAY subquery and MULTISET/MAP subquery. Please refer to the comments of the CALCITE-6063 for details. We may decide to make some improvements in follow-up issues or PRs.

@chucheng92 chucheng92 changed the title [CALCITE-6063] If ARRAY subquery has ORDER BY(without LIMIT), rows are not sorted [CALCITE-6063] If ARRAY subquery has ORDER BY (without LIMIT), rows are not sorted Jan 25, 2024
@chucheng92
Copy link
Member Author

chucheng92 commented Jan 26, 2024

Hi guys. Sorry, I accidentally forced-push PR instead of a new commit, but there are no new lines affecting the logic, only the test cases for map/multiset.

If there are no other comments, I will merge it later. Thanks for reviewing it!

@chucheng92
Copy link
Member Author

merging...

@chucheng92 chucheng92 merged commit dc4ce8b into apache:main Jan 27, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants