[CALCITE-5510] Support sort by ordinal when sort by column is an expression in RelToSqlConverter #3057

peng128 · 2023-02-02T09:44:54Z

Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

…ession in RelToSqlConverter Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

zoudan · 2023-02-06T06:39:32Z

LGTM

chunweilei · 2023-02-14T03:12:56Z

core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java

@@ -759,17 +760,17 @@ private static String toSql(RelNode root, SqlDialect dialect,
    final String expected = "SELECT \"product_class_id\", COUNT(*) AS \"C\"\n"
        + "FROM \"foodmart\".\"product\"\n"
        + "GROUP BY ROLLUP(\"product_class_id\")\n"
-        + "ORDER BY \"product_class_id\", COUNT(*)";
+        + "ORDER BY \"product_class_id\", 2";
    final String expectedMysql = "SELECT `product_class_id`, COUNT(*) AS `C`\n"
        + "FROM `foodmart`.`product`\n"


COUNT(*) is replaced by 2 rather than 1. So the ordinal starts from 1?

Yes, I think the ordinal is the position of column in select list, it starts from 1.

libenchao

@peng128 thanks for your PR, I've left some comments.

libenchao · 2023-02-19T12:27:22Z

core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java

    final String expectedMysql = "SELECT `product_class_id`, `brand_name`,"
        + " COUNT(*) AS `C`\n"
        + "FROM `foodmart`.`product`\n"
        + "GROUP BY `product_class_id`, `brand_name` WITH ROLLUP\n"
        + "ORDER BY `product_class_id` IS NULL, `product_class_id`,"
        + " `brand_name` IS NULL, `brand_name`,"
-        + " COUNT(*) IS NULL, COUNT(*)";
+        + " 3 IS NULL, 3";


Is order by 3 is null valid here? In Calcite, we'll take this not a 'field reference'. I'm not sure how other databases behaves in this case.

I think it valid here.
I try to make a test to compare the relNode compare '3 is null' sql with the original SQL and the 'count(*) is null' sql.
Using default SqlParser config.

For original sql:

select "product_class_id", "brand_name",count(*) as c from "product" group by rollup("product_class_id", "brand_name") order by 1, 2, 3

relNode is

LogicalSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC]) LogicalAggregate(group=[{0, 1}], groups=[[{0, 1}, {0}, {}]], C=[COUNT()]) LogicalProject(product_class_id=[$0], brand_name=[$2]) JdbcTableScan(table=[[foodmart, product]])

For 'count(*) is null' sql:

select "product_class_id", "brand_name", count(*) as c from "product" group by rollup("product_class_id", "brand_name") order by "product_class_id", "brand_name", count(*) is null, count(*)

relNode is

LogicalProject(product_class_id=[$0], brand_name=[$1], C=[$2]) LogicalSort(sort0=[$0], sort1=[$1], sort2=[$3], sort3=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC], dir3=[ASC]) LogicalProject(product_class_id=[$0], brand_name=[$1], C=[$2], EXPR$3=[false]) LogicalAggregate(group=[{0, 1}], groups=[[{0, 1}, {0}, {}]], C=[COUNT()]) LogicalProject(product_class_id=[$0], brand_name=[$2]) JdbcTableScan(table=[[foodmart, product]])

For '3 is null' sql:

select "product_class_id", "brand_name", count(*) as c from "product" group by rollup("product_class_id", "brand_name") order by 1, 2, 3 is null, 3

relNode is

LogicalProject(product_class_id=[$0], brand_name=[$1], C=[$2]) LogicalSort(sort0=[$0], sort1=[$1], sort2=[$3], sort3=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC], dir3=[ASC]) LogicalProject(product_class_id=[$0], brand_name=[$1], C=[$2], EXPR$3=[false]) LogicalAggregate(group=[{0, 1}], groups=[[{0, 1}, {0}, {}]], C=[COUNT()]) LogicalProject(product_class_id=[$0], brand_name=[$2]) JdbcTableScan(table=[[foodmart, product]])

The relNode of '3 is null' sql and 'count(*) is null' sql is almost some. Is my understanding correct?
So I think it's fine here. And I test '3 is null' sql in MySQL, it works.

No, what I mean is that we only takes pure literal as a field reference in Calcite, if it's an expression, it won't be taken as a field reference. Your second case shows what I described above.

To be more precise, 3 is null equals false, not count(*) is null in this case.

I'm not saying Calcite is definitely correct for current behavior. We need to investigate what other databases behave when the literal is in an expression, and let Calcite be consistent with the majority of them.

If we prefer to not take literal in expressions as a field reference, then your change now would make it incorrect.

I got it. We shouldn't take literal in experssion as field reference.
How about keep original expression when we need to sort null values in order by?
For example, the output sql will be

select "product_class_id", "brand_name", count(*) as c from "product" group by rollup("product_class_id", "brand_name") order by 1, 2, count(*) is null, 3

This sounds good to me.

I fixed it. Please review again, thanks.

libenchao · 2023-02-19T12:27:41Z

core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java

@@ -615,6 +615,13 @@ public SqlNode orderField(int ordinal) {
        final String strValue = ((SqlNumericLiteral) node).toValue();
        return SqlLiteral.createCharString(strValue, node.getParserPosition());
      }
+      if (node instanceof SqlCall
+          && dialect.getConformance().isSortByOrdinal()) {
+        // If the filed is expression and sort by ordinal is set in dialect,


filed -> field

Fixed it. Thanks~

…ession in RelToSqlConverter Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

libenchao

Looks good to me now, with one minor comment. After fixing that, I'll merge this PR.

libenchao · 2023-02-20T11:47:12Z

core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java

+   * RelToSqlConverter don't support sort by ordinal when sort by column is an expression</a>.
+   */
+  @Test void testOrderByOrdinalWithExpression() {
+    String query = "select \"product_id\", count(*) as \"c\"\n"


…ession in RelToSqlConverter Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

libenchao · 2023-02-20T13:16:51Z

reopened, because slow tests fail due to this change: https://github.com/apache/calcite/actions/runs/4223301074/jobs/7332876394

…ession in RelToSqlConverter Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

sonarqubecloud · 2023-02-21T12:14:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

93.0% Coverage
0.0% Duplication

libenchao · 2023-02-21T14:31:04Z

core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java

+          && dialect.getConformance().isSortByOrdinal()) {
+        // Cannot merge a Project that contains sort by ordinal under it.
+        return hasSortByOrdinal();
+      }


The last version we merged does not have this logic, could you help me to understand why do you need to add this logic now?

We found test case LatticeTest#testSum is failed in slow test.
These code is to fix it.

The reason of test case failure is that the project will be merged when the sub-relnode is sort(change by CALCITE-4901).

For example,
If the relNode is

JdbcProject(C=[$1]) JdbcSort(sort0=[$1], dir0=[DESC], fetch=[1]) JdbcAggregate(group=[{0}], C=[$SUM0($7)]) JdbcTableScan(table=[[foodmart, sales_fact_1997]])

The result after merge project is

SELECT SUM(`sales_fact_1997`.`unit_sales`) AS `C` FROM `foodmart`.`sales_fact_1997` AS `sales_fact_1997` GROUP BY `sales_fact_1997`.`product_id` ORDER BY 2 DESC

it's a incorrect sql, we want

SELECT `C` FROM ( SELECT `sales_fact_1997`.`product_id`, SUM(`sales_fact_1997`.`unit_sales`) AS `C` FROM `foodmart`.`sales_fact_1997` AS `sales_fact_1997` GROUP BY `sales_fact_1997`.`product_id` ORDER BY 2 DESC) t

So, these code to find out whether there is sort by original under project.
And don't merge project when there is sort by original under project.

Since we agreed that the slow tests failure is not caused by this PR, we'd better handle this in CALCITE-5537.

There are three test cases failure.

LatticeTest#testSum

LatticeSuggesterTest#testFoodMartAll

LatticeSuggesterTest#testFoodMartAllEvolve

Case 1 is caused by this PR. Maybe we could fix in this PR?
Case 2 & 3 is not caused by this PR. I think we could fix case 2 & 3 in CALCITE-5537.

BTW, these code just fix case 1.

Thanks for the clarification, I agree with you. I'll review the PR in the coming days.

libenchao

+1, merging. Thanks @peng128 for your contribution!

… the dialect allows Close apache#3057 (cherry picked from commit a990ecc)

peng128 marked this pull request as draft February 2, 2023 10:57

[CALCITE-5510] Support sort by ordinal when sort by column is an expr…

a023127

…ession in RelToSqlConverter Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

peng128 force-pushed the CALCITE-5510 branch from a46a382 to a023127 Compare February 2, 2023 13:04

peng128 marked this pull request as ready for review February 2, 2023 13:18

chunweilei reviewed Feb 14, 2023

View reviewed changes

libenchao reviewed Feb 19, 2023

View reviewed changes

wangpeng added 2 commits February 19, 2023 22:07

[CALCITE-5510] Support sort by ordinal when sort by column is an expr…

354c17d

…ession in RelToSqlConverter Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

[CALCITE-5510] Support sort by ordinal when sort by column is an expr…

081f43e

…ession in RelToSqlConverter Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

libenchao reviewed Feb 20, 2023

View reviewed changes

[CALCITE-5510] Support sort by ordinal when sort by column is an expr…

55b0f48

…ession in RelToSqlConverter Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

libenchao closed this in 0e8d6b0 Feb 20, 2023

libenchao reopened this Feb 20, 2023

wangpeng added 2 commits February 21, 2023 16:38

[CALCITE-5510] Support sort by ordinal when sort by column is an expr…

88622c1

…ession in RelToSqlConverter Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

[CALCITE-5510] Support sort by ordinal when sort by column is an expr…

2cf5413

…ession in RelToSqlConverter Keep the output sort by column to ordinary when sort by ordinal in dialect is true and the column is an expression.

libenchao reviewed Feb 21, 2023

View reviewed changes

libenchao approved these changes Feb 25, 2023

View reviewed changes

libenchao closed this in a990ecc Feb 25, 2023

eventd pushed a commit to Kyligence/calcite that referenced this pull request Mar 3, 2023

[CALCITE-5510] RelToSqlConverter should use ordinal for ORDER BY if…

593b32b

… the dialect allows Close apache#3057 (cherry picked from commit a990ecc)

libenchao mentioned this pull request Apr 30, 2023

[CALCITE-5530] RelToSqlConverter[ORDER BY] generates an incorrect fie… #3085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-5510] Support sort by ordinal when sort by column is an expression in RelToSqlConverter #3057

[CALCITE-5510] Support sort by ordinal when sort by column is an expression in RelToSqlConverter #3057

peng128 commented Feb 2, 2023

zoudan commented Feb 6, 2023

chunweilei Feb 14, 2023

peng128 Feb 14, 2023

libenchao left a comment

libenchao Feb 19, 2023

peng128 Feb 19, 2023

libenchao Feb 20, 2023 •

edited

Loading

peng128 Feb 20, 2023

libenchao Feb 20, 2023

peng128 Feb 20, 2023

libenchao Feb 19, 2023

peng128 Feb 19, 2023

libenchao left a comment

libenchao Feb 20, 2023

libenchao commented Feb 20, 2023

sonarqubecloud bot commented Feb 21, 2023

libenchao Feb 21, 2023

peng128 Feb 22, 2023

libenchao Feb 22, 2023

peng128 Feb 22, 2023

libenchao Feb 24, 2023

libenchao left a comment

[CALCITE-5510] Support sort by ordinal when sort by column is an expression in RelToSqlConverter #3057

[CALCITE-5510] Support sort by ordinal when sort by column is an expression in RelToSqlConverter #3057

Conversation

peng128 commented Feb 2, 2023

zoudan commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libenchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libenchao Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libenchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libenchao commented Feb 20, 2023

sonarqubecloud bot commented Feb 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libenchao left a comment

Choose a reason for hiding this comment

libenchao Feb 20, 2023 •

edited

Loading