[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias #1715

helloppx · 2020-01-02T08:17:01Z

https://issues.apache.org/jira/browse/CALCITE-3662

…regate)) and aggregate field has no alias (Lei Jiang)

helloppx · 2020-01-02T08:38:15Z

piglet/src/test/java/org/apache/calcite/test/PigRelOpTest.java

-        + "                  ORDER BY SAL) AS t5\n"
-        + "            GROUP BY 'all') AS t8) AS $cor5,\n"
+        + "                  GROUP BY 'all'\n"
+        + "                  ORDER BY SAL) AS t7) AS t8) AS $cor5,\n"
        + "    LATERAL UNNEST (SELECT $cor5.X AS $f0\n"


@khaitranq @ihuzenko @danny0405 @vlsi
Could you help me review this test case ?

Currently select list is given as '*' when wrapSelect [1], however it's seems not right for this case.
Shall we set the correct alias for such case ?
I think manipulate the internals of builder is not a good idea.

[1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java#L437

Background info:
Because SELECT is less than FETCH[1], when RelToSqlConverter visits Project, it will wrap Sort(Aggregate) as a subselect. When aggregate field has no alias, the select list of Project will be wrong.

Hi Jin Xing, I referenced method visitAggregate[2], it meets the same problem.

[1]
//Clauses in a SQL query. Ordered by evaluation order.
//SELECT is set only when there is a NON-TRIVIAL SELECT clause
public enum Clause {
FROM, WHERE, GROUP_BY, HAVING, SELECT, SET_OP, ORDER_BY, FETCH, OFFSET
}
[2]
https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java#L289

Thanks for explanation @helloppx
Yes, thee reference is indeed a fix for this case. But I still think a thorough fix is to set the correct alias. I take below case as an example:
If I have sql and corresponding plan as below:

select "product_class_id" + 1, * from ( select * from (select "product_class_id", sum("product_id") from "product" group by "product_class_id" limit 10) A union all select * from (select "product_class_id", sum("product_id") from "product" group by "product_class_id" limit 10) B ) C LogicalProject(EXPR$0=[+($0, 1)], product_class_id=[$0], EXPR$1=[$1]) LogicalUnion(all=[true]) LogicalProject(product_class_id=[$0], EXPR$1=[$1]) LogicalSort(fetch=[10]) LogicalAggregate(group=[{0}], EXPR$1=[SUM($1)]) LogicalProject(product_class_id=[$0], product_id=[$1]) JdbcTableScan(table=[[foodmart, product]]) LogicalProject(product_class_id=[$0], EXPR$1=[$1]) LogicalSort(fetch=[10]) LogicalAggregate(group=[{0}], EXPR$1=[SUM($1)]) LogicalProject(product_class_id=[$0], product_id=[$1]) JdbcTableScan(table=[[foodmart, product]])

RelToSqlConverter will convert the plan to below sql (with the fix bring by this PR):

SELECT "product_class_id" + 1, "product_class_id", SUM("product_id") FROM (SELECT "product_class_id", SUM("product_id") FROM "foodmart"."product" GROUP BY "product_class_id" FETCH NEXT 10 ROWS ONLY UNION ALL SELECT "product_class_id", SUM("product_id") FROM "foodmart"."product" GROUP BY "product_class_id" FETCH NEXT 10 ROWS ONLY) AS "t5"

Obviously it's not correct (the outmost SUM("product_id") is wrong).

If we rectify the alias for subquery, above case I mentioned should work well.

jinxing64 · 2020-01-02T12:26:50Z

How about add a test in RelToSqlConverterTest ?

helloppx · 2020-01-03T07:05:34Z

How about add a test in RelToSqlConverterTest ?

I did some research. If we want to reproduce this JIRA case, we must use SqlToRelConverter to convert SQL to REL and then use RelToSqlConverter to generate SQL. If we move this case to RelToSqlConverterTest, It is not easy to use SqlToRelTestBase#createTester()#convertSqlToRel(sql) method for this case.

…regate)) and aggregate field has no alias (Lei Jiang)

helloppx · 2020-01-06T11:08:43Z

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

+        node = as(node, name);
+      } else {
+        ordinalMap.put(lowerName, node);
+      }


Hi @jinxing64 , I added a test case you mentioned: testSelectImplicitAliasByStar
Above change(5 lines) can fix that issue, but I don't know whether it is a good idea, since we have to update select-list of 12 test cases.

Failed tests
Classes
RelToSqlConverterTest. testDb2DialectSelectQueryComplex()
RelToSqlConverterTest. testExceptOperatorForBigQuery()
RelToSqlConverterTest. testExistsWithExpand()
RelToSqlConverterTest. testIntersectOperatorForBigQuery()
RelToSqlConverterTest. testNotExistsWithExpand()
RelToSqlConverterTest. testSelectQueryComplex()
RelToSqlConverterTest. testSelectQueryWithGroupByHaving3()
RelToSqlConverterTest. testUncollectImplicitAlias()
RelToSqlConverterTest. testUnionAllOperatorForBigQuery()
RelToSqlConverterTest. testUnionAllWithNoOperands()
RelToSqlConverterTest. testUnionAllWithNoOperandsUsingOracleDialect()
RelToSqlConverterTest. testUnionOperatorForBigQuery()

What isordinalMap used for ?

Hi @helloppx ~
Thanks a lot for digging into this ~
Are you sure all the failed tests still generate the correct sql by semantics ?

Yes that 12 cases are semantically correct

ordinalMap is a "EXPR$x" -> "EXPRESSION" store: EXPR$1 -> product_id + 1
Subquery save the kv to ordinalMap and then outer query call AliasContext#field() to fill select-list by ordinalMap.

jinxing64 · 2020-01-06T13:48:38Z

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

+        node = as(node, name);
+      } else {
+        ordinalMap.put(lowerName, node);
+      }


What isordinalMap used for ?

jinxing64 · 2020-01-06T13:50:20Z

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

@@ -919,7 +924,11 @@ public Result visit(TableFunctionScan e) {
    final String lowerName = name.toLowerCase(Locale.ROOT);
    if (lowerName.startsWith("expr$")) {
      // Put it in ordinalMap
-      ordinalMap.put(lowerName, node);
+      if (stack.size() != 1) {
+        node = as(node, name);


I understand this as that we don't need to add an alias for the outmost node.

The semantic is correct, but, can we have a more simple solution for the SQL, the RelToSqlConverterTest has many failing test cases(some of which is correct to fix but most are unnecessary change)

Agree with Danny, +1

jinxing64 · 2020-01-06T13:52:03Z

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

+        node = as(node, name);
+      } else {
+        ordinalMap.put(lowerName, node);
+      }


Hi @helloppx ~
Thanks a lot for digging into this ~
Are you sure all the failed tests still generate the correct sql by semantics ?

jinxing64 · 2020-01-06T13:56:45Z

core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java

+            "SELECT \"product_id\", COALESCE(SUM(\"store_sales\"), 0) AS \"EXPR$1\"\n" +
+            "FROM \"foodmart\".\"sales_fact_1997\"\n" +
+            "GROUP BY \"product_id\") AS \"t1\"");
+  }


Another simpler unit test as below

select "shelf_width", * from (select "shelf_width", "product_id" + 1 from "product" union all select "shelf_width", "product_id" from "product") A

But it converted as

SELECT "shelf_width", "shelf_width" AS "shelf_width0", "product_id" + 1 FROM (SELECT "shelf_width", "product_id" + 1 FROM "foodmart"."product" UNION ALL SELECT "shelf_width", "product_id" FROM "foodmart"."product") AS "t1"

As we can see, above conversion is not correct.

I tested that case and it seems right:

SELECT "shelf_width", "shelf_width" AS "shelf_width0", "EXPR$1" FROM (SELECT "shelf_width", "product_id" + 1 AS "EXPR$1" FROM "foodmart"."product" UNION ALL SELECT "shelf_width", "product_id" FROM "foodmart"."product") AS "t1"

danny0405 · 2020-01-07T08:10:02Z

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

+    if (e.getInput() instanceof Sort) {
+      builder = x.builder(e);
+      builder.clauses.add(Clause.SELECT);
+    } else {


builder.clauses.add(Clause.SELECT) is useless.

With this change, i believe that code snippet in visit(Sort) can be removed:

if (stack.size() != 1 && builder.select.getSelectList() == null) { // Generates explicit column names instead of start(*) for // non-root order by to avoid ambiguity. final List<SqlNode> selectList = Expressions.list(); for (RelDataTypeField field : e.getRowType().getFieldList()) { addSelect(selectList, builder.context.field(field.getIndex()), e.getRowType()); } builder.select.setSelectList(new SqlNodeList(selectList, POS)); }

I think below fix snippet here is not correct.

if (e.getInput() instanceof Sort) { builder = x.builder(e); builder.clauses.add(Clause.SELECT); } else { builder = x.builder(e, Clause.SELECT); }

Above change apply the projects on the select list from Sort. If Sort uses ordinal in ORDER BY clause, the semantics can be wrong.
Check below case

select "p" * (-1) from (select "product_id" "p", sum("net_weight") "product_id" from "product" group by "product_id" order by 1 limit 10) A

It will be converted as

SELECT \"product_id\" * -1 FROM \"foodmart\".\"product\" GROUP BY \"product_id\" ORDER BY 1 FETCH NEXT 10 ROWS ONLY

The order of the result is obviously inconsistent with the original Sql.

If we fix the alias, I think we don't need the change here.

We may need to replace the order by ordinal with order by alias to solve this problem completely. Not all the DBs supports order by ordinal, they all support order by alias.

Thank you very much @jinxing64 , @danny0405
I did some test, it will cause another issue[1] to replace order by ordinal with order by alias,
so I deleted the code snippet[2]

[1]

//SqlImplementor#Result //replace `order by ordinal` with `order by alias` @Override public SqlNode orderField(int ordinal) { final SqlNode node = field(ordinal); if (node instanceof SqlIdentifier && ((SqlIdentifier) node).isSimple()) { Result result = Result.this; final String alias = SqlValidatorUtil.getAlias(selectList.get(ordinal), -1); return new SqlIdentifier(ImmutableList.of(alias), POS); } return node; } //The above method and [2] will produce wrong SQL SELECT \"product_id\" * -1 FROM \"foodmart\".\"product\" GROUP BY \"product_id\" ORDER BY \"p\" FETCH NEXT 10 ROWS ONLY

[2]

if (e.getInput() instanceof Sort) { builder = x.builder(e); } else { builder = x.builder(e, Clause.SELECT); }

danny0405 · 2020-01-07T08:12:07Z

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

@@ -919,7 +924,11 @@ public Result visit(TableFunctionScan e) {
    final String lowerName = name.toLowerCase(Locale.ROOT);
    if (lowerName.startsWith("expr$")) {
      // Put it in ordinalMap
-      ordinalMap.put(lowerName, node);
+      if (stack.size() != 1) {
+        node = as(node, name);


The semantic is correct, but, can we have a more simple solution for the SQL, the RelToSqlConverterTest has many failing test cases(some of which is correct to fix but most are unnecessary change)

danny0405 · 2020-01-07T08:12:30Z

core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java

+        .query("select sum(sal)\n"
+            + "from scott.emp \n"
+            + "group by ename limit 10")
+        .planHasSql("SELECT SUM(\"SAL\")\n"


You should add the tests in RelToSqlConverterTest.

yanlin-Lynn · 2020-01-08T03:01:36Z

I add a test case in RelToSqlConverterTest, it seems to produce a correct sql

@Test public void testGroupByWithLimitPlan() {
    String query = "SELECT sum(\"salary\") FROM \"employee\" group by \"gender\" limit 10";
    sql(query)
        .ok("SELECT SUM(\"salary\")\n"
            + "FROM \"foodmart\".\"employee\"\n"
            + "GROUP BY \"gender\"\n"
            + "FETCH NEXT 10 ROWS ONLY");
  }

…regate)) and aggregate field has no alias (Lei Jiang)

helloppx · 2020-01-08T07:35:46Z

I add a test case in RelToSqlConverterTest, it seems to produce a correct sql

@Test public void testGroupByWithLimitPlan() {
    String query = "SELECT sum(\"salary\") FROM \"employee\" group by \"gender\" limit 10";
    sql(query)
        .ok("SELECT SUM(\"salary\")\n"
            + "FROM \"foodmart\".\"employee\"\n"
            + "GROUP BY \"gender\"\n"
            + "FETCH NEXT 10 ROWS ONLY");
  }

Hi @yanlin-Lynn @danny0405
If we want to reproduce this JIRA case we need to make sure Sort is input of Project [1].
If we move the case to RelToSqlConverterTest, it will produce a plan [2] so we cannot reproduce the JIRA case.

[1]

JdbcToEnumerableConverter
  JdbcProject(EXPR$0=[$1])
    JdbcSort(fetch=[10])
      JdbcAggregate(group=[{1}], EXPR$0=[SUM($5)])
        JdbcTableScan(table=[[SCOTT, EMP]])

[2]

LogicalSort(fetch=[10])
  LogicalProject(EXPR$0=[$1])
    LogicalAggregate(group=[{0}], EXPR$0=[SUM($1)])
      LogicalProject(brand_name=[$2], net_weight=[$7])
        JdbcTableScan(table=[[foodmart, product]])

yanlin-Lynn · 2020-01-08T08:21:48Z

Currently, I'm using RelToSqlConverter in my project, so I spent some time on this.
I tried to fix this in buildAggregate, and it's affected 5 test cases in RelToSqlConverterTest.
Here is the prototype of my fix, https://github.com/yanlin-Lynn/calcite/tree/CALCITE-3662, hope it's of some use to you.

danny0405 · 2020-01-08T08:40:20Z

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

+      clone.pop();
+      final Frame parent = clone.peek();
+      if (parent != null && parent.r instanceof Union) {
+        // For case: Project(Union(expression without alias))


Not only Union, i believe all the SetOp and BiRel can have such a simplification.

Hi @danny0405 , I have updated the solution and fix the code conflict.

…regate)) and aggregate field has no alias (Lei Jiang)

danny0405 · 2020-01-09T08:31:40Z

Can you fix the code conflict ?

…regate)) and aggregate field has no alias (Lei Jiang)

yanlin-Lynn · 2020-01-09T09:14:44Z

We can reproduce the case in RelToSqlConverterTest, you can apply SortProjectTransposeRule.INSTANCE rule to make sure Sort transpose Project

…regate)) and aggregate field has no alias (Lei Jiang)

helloppx · 2020-01-09T09:52:46Z

Currently, I'm using RelToSqlConverter in my project, so I spent some time on this.
I tried to fix this in buildAggregate, and it's affected 5 test cases in RelToSqlConverterTest.
Here is the prototype of my fix, https://github.com/yanlin-Lynn/calcite/tree/CALCITE-3662, hope it's of some use to you.

Thank you @yanlin-Lynn , I have moved the test case. :D

…regate)) and aggregate field has no alias (Lei Jiang)

[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Agg…

2d0335a

…regate)) and aggregate field has no alias (Lei Jiang)

helloppx commented Jan 2, 2020

View reviewed changes

[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Agg…

e3fddf2

…regate)) and aggregate field has no alias (Lei Jiang)

helloppx commented Jan 6, 2020

View reviewed changes

jinxing64 reviewed Jan 6, 2020

View reviewed changes

danny0405 reviewed Jan 7, 2020

View reviewed changes

[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Agg…

48cb514

…regate)) and aggregate field has no alias (Lei Jiang)

danny0405 reviewed Jan 8, 2020

View reviewed changes

[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Agg…

4c05dd9

…regate)) and aggregate field has no alias (Lei Jiang)

[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Agg…

e6943c9

…regate)) and aggregate field has no alias (Lei Jiang)

helloppx added 2 commits January 9, 2020 17:35

[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Agg…

ba08a0c

…regate)) and aggregate field has no alias (Lei Jiang)

[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Agg…

c6c1e72

…regate)) and aggregate field has no alias (Lei Jiang)

[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Agg…

bfc7246

…regate)) and aggregate field has no alias (Lei Jiang)

helloppx closed this Jan 9, 2020

helloppx reopened this Jan 9, 2020

hsyuan force-pushed the master branch from e42a79b to a0ef3c9 Compare March 29, 2020 16:52

vlsi force-pushed the master branch from e4458fa to 5462be9 Compare July 17, 2020 20:19

vlsi force-pushed the master branch from fd20efd to 3311d45 Compare December 9, 2020 20:10

julianhyde force-pushed the master branch from 52c1284 to d4e1eea Compare March 1, 2021 02:56

vlsi force-pushed the master branch from 7f65cf2 to 4bc9166 Compare March 24, 2021 09:43

zabetak force-pushed the master branch from f14cf4c to dcbc493 Compare March 10, 2022 09:13

julianhyde force-pushed the main branch from fa65a2e to 1226d1a Compare June 20, 2022 20:27

asfgit force-pushed the main branch from 9fc50f2 to e2f949d Compare September 10, 2022 16:37

asfgit force-pushed the main branch from f8f8a51 to a326bd2 Compare January 25, 2023 07:39

julianhyde force-pushed the main branch 2 times, most recently from 8a5cf83 to cf7f71b Compare June 8, 2023 21:21

tanclary force-pushed the main branch from 4804912 to 00db001 Compare September 6, 2023 00:42

libenchao force-pushed the main branch from 47db81a to 0be8eae Compare November 10, 2023 13:18

[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias #1715

Are you sure you want to change the base?

[CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias #1715

Conversation

helloppx commented Jan 2, 2020

Choose a reason for hiding this comment

jinxing64 Jan 2, 2020 • 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

jinxing64 commented Jan 2, 2020

helloppx commented Jan 3, 2020

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

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

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

helloppx Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanlin-Lynn commented Jan 8, 2020

helloppx commented Jan 8, 2020

yanlin-Lynn commented Jan 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 commented Jan 9, 2020

yanlin-Lynn commented Jan 9, 2020

helloppx commented Jan 9, 2020

jinxing64 Jan 2, 2020 •

edited

Loading

helloppx Jan 9, 2020 •

edited

Loading