You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/01/02 08:17:02 UTC
[GitHub] [calcite] helloppx opened a new pull request #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
helloppx opened a new pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715
[https://issues.apache.org/jira/browse/CALCITE-3662](https://issues.apache.org/jira/browse/CALCITE-3662)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on issue #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on issue #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#issuecomment-570193675
How about add a test in `RelToSqlConverterTest` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r362756121
##########
File path: piglet/src/test/java/org/apache/calcite/test/PigRelOpTest.java
##########
@@ -468,14 +468,14 @@ private Fluent pig(String script) {
+ "HIREDATE, SAL, COMM, DEPTNO)) AS A\n"
+ " FROM scott.EMP\n"
+ " GROUP BY DEPTNO) AS $cor4,\n"
- + " LATERAL (SELECT COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
- + " FROM (SELECT ENAME, JOB, DEPTNO, SAL\n"
+ + " LATERAL (SELECT X\n"
+ + " FROM (SELECT 'all' AS $f0, COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
+ " FROM UNNEST (SELECT $cor4.A AS $f0\n"
+ " FROM (VALUES (0)) AS t (ZERO)) "
+ "AS t2 (EMPNO, ENAME, JOB, MGR, HIREDATE, SAL, COMM, DEPTNO)\n"
+ " WHERE JOB <> 'CLERK'\n"
- + " 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"
Review comment:
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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r364074197
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -227,8 +227,13 @@ public Result visit(Project e) {
if (isStar(e.getChildExps(), e.getInput().getRowType(), e.getRowType())) {
return x;
}
- final Builder builder =
- x.builder(e, Clause.SELECT);
+ final Builder builder;
+ if (e.getInput() instanceof Sort) {
+ builder = x.builder(e);
+ builder.clauses.add(Clause.SELECT);
+ } else {
Review comment:
If we fix the alias, I think we don't need the change here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on issue #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on issue #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#issuecomment-571927284
> 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]])
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on issue #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on issue #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#issuecomment-570484167
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r364074249
##########
File path: 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);
Review comment:
Agree with Danny, +1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363300215
##########
File path: 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);
Review comment:
I understand this as that we don't need to add an alias for the outmost node.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r364115815
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -227,8 +227,13 @@ public Result visit(Project e) {
if (isStar(e.getChildExps(), e.getInput().getRowType(), e.getRowType())) {
return x;
}
- final Builder builder =
- x.builder(e, Clause.SELECT);
+ final Builder builder;
+ if (e.getInput() instanceof Sort) {
+ builder = x.builder(e);
+ builder.clauses.add(Clause.SELECT);
+ } else {
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r362399146
##########
File path: piglet/src/test/java/org/apache/calcite/test/PigRelOpTest.java
##########
@@ -468,14 +468,14 @@ private Fluent pig(String script) {
+ "HIREDATE, SAL, COMM, DEPTNO)) AS A\n"
+ " FROM scott.EMP\n"
+ " GROUP BY DEPTNO) AS $cor4,\n"
- + " LATERAL (SELECT COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
- + " FROM (SELECT ENAME, JOB, DEPTNO, SAL\n"
+ + " LATERAL (SELECT X\n"
+ + " FROM (SELECT 'all' AS $f0, COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
+ " FROM UNNEST (SELECT $cor4.A AS $f0\n"
+ " FROM (VALUES (0)) AS t (ZERO)) "
+ "AS t2 (EMPNO, ENAME, JOB, MGR, HIREDATE, SAL, COMM, DEPTNO)\n"
+ " WHERE JOB <> 'CLERK'\n"
- + " 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"
Review comment:
@khaitranq @ihuzenko @danny0405 @vlsi
Could you help me review this test case ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx closed pull request #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx closed pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r364115101
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -918,8 +913,16 @@ public Result visit(TableFunctionScan e) {
String alias = SqlValidatorUtil.getAlias(node, -1);
final String lowerName = name.toLowerCase(Locale.ROOT);
if (lowerName.startsWith("expr$")) {
- // Put it in ordinalMap
- ordinalMap.put(lowerName, node);
+ final ArrayDeque<Frame> clone = new ArrayDeque<>(stack);
+ clone.pop();
+ final Frame parent = clone.peek();
+ if (parent != null && parent.r instanceof Union) {
+ // For case: Project(Union(expression without alias))
Review comment:
Not only `Union`, i believe all the `SetOp` and `BiRel` can have such a simplification.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363601259
##########
File path: 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);
+ } else {
+ ordinalMap.put(lowerName, node);
+ }
Review comment:
`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`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] yanlin-Lynn commented on issue #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on issue #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#issuecomment-571940496
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r364653745
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -227,8 +227,13 @@ public Result visit(Project e) {
if (isStar(e.getChildExps(), e.getInput().getRowType(), e.getRowType())) {
return x;
}
- final Builder builder =
- x.builder(e, Clause.SELECT);
+ final Builder builder;
+ if (e.getInput() instanceof Sort) {
+ builder = x.builder(e);
+ builder.clauses.add(Clause.SELECT);
+ } else {
Review comment:
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);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r362756548
##########
File path: piglet/src/test/java/org/apache/calcite/test/PigRelOpTest.java
##########
@@ -468,14 +468,14 @@ private Fluent pig(String script) {
+ "HIREDATE, SAL, COMM, DEPTNO)) AS A\n"
+ " FROM scott.EMP\n"
+ " GROUP BY DEPTNO) AS $cor4,\n"
- + " LATERAL (SELECT COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
- + " FROM (SELECT ENAME, JOB, DEPTNO, SAL\n"
+ + " LATERAL (SELECT X\n"
+ + " FROM (SELECT 'all' AS $f0, COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
+ " FROM UNNEST (SELECT $cor4.A AS $f0\n"
+ " FROM (VALUES (0)) AS t (ZERO)) "
+ "AS t2 (EMPNO, ENAME, JOB, MGR, HIREDATE, SAL, COMM, DEPTNO)\n"
+ " WHERE JOB <> 'CLERK'\n"
- + " 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"
Review comment:
If we rectify the alias for subquery, above case I mentioned should work well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363601885
##########
File path: 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);
+ } else {
+ ordinalMap.put(lowerName, node);
+ }
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r362456472
##########
File path: piglet/src/test/java/org/apache/calcite/test/PigRelOpTest.java
##########
@@ -468,14 +468,14 @@ private Fluent pig(String script) {
+ "HIREDATE, SAL, COMM, DEPTNO)) AS A\n"
+ " FROM scott.EMP\n"
+ " GROUP BY DEPTNO) AS $cor4,\n"
- + " LATERAL (SELECT COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
- + " FROM (SELECT ENAME, JOB, DEPTNO, SAL\n"
+ + " LATERAL (SELECT X\n"
+ + " FROM (SELECT 'all' AS $f0, COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
+ " FROM UNNEST (SELECT $cor4.A AS $f0\n"
+ " FROM (VALUES (0)) AS t (ZERO)) "
+ "AS t2 (EMPNO, ENAME, JOB, MGR, HIREDATE, SAL, COMM, DEPTNO)\n"
+ " WHERE JOB <> 'CLERK'\n"
- + " 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"
Review comment:
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363581275
##########
File path: 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);
+ } else {
+ ordinalMap.put(lowerName, node);
+ }
Review comment:
Yes that 12 cases are semantically correct
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363299547
##########
File path: 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);
+ } else {
+ ordinalMap.put(lowerName, node);
+ }
Review comment:
What is`ordinalMap` used for ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363631241
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -227,8 +227,13 @@ public Result visit(Project e) {
if (isStar(e.getChildExps(), e.getInput().getRowType(), e.getRowType())) {
return x;
}
- final Builder builder =
- x.builder(e, Clause.SELECT);
+ final Builder builder;
+ if (e.getInput() instanceof Sort) {
+ builder = x.builder(e);
+ builder.clauses.add(Clause.SELECT);
+ } else {
Review comment:
With this change, i believe that code snippet in `visit(Sort)` can be removed:
```java
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));
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r364647262
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -918,8 +913,16 @@ public Result visit(TableFunctionScan e) {
String alias = SqlValidatorUtil.getAlias(node, -1);
final String lowerName = name.toLowerCase(Locale.ROOT);
if (lowerName.startsWith("expr$")) {
- // Put it in ordinalMap
- ordinalMap.put(lowerName, node);
+ final ArrayDeque<Frame> clone = new ArrayDeque<>(stack);
+ clone.pop();
+ final Frame parent = clone.peek();
+ if (parent != null && parent.r instanceof Union) {
+ // For case: Project(Union(expression without alias))
Review comment:
Hi @danny0405 , I have updated the solution and fix the code conflict.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r362456472
##########
File path: piglet/src/test/java/org/apache/calcite/test/PigRelOpTest.java
##########
@@ -468,14 +468,14 @@ private Fluent pig(String script) {
+ "HIREDATE, SAL, COMM, DEPTNO)) AS A\n"
+ " FROM scott.EMP\n"
+ " GROUP BY DEPTNO) AS $cor4,\n"
- + " LATERAL (SELECT COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
- + " FROM (SELECT ENAME, JOB, DEPTNO, SAL\n"
+ + " LATERAL (SELECT X\n"
+ + " FROM (SELECT 'all' AS $f0, COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
+ " FROM UNNEST (SELECT $cor4.A AS $f0\n"
+ " FROM (VALUES (0)) AS t (ZERO)) "
+ "AS t2 (EMPNO, ENAME, JOB, MGR, HIREDATE, SAL, COMM, DEPTNO)\n"
+ " WHERE JOB <> 'CLERK'\n"
- + " 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"
Review comment:
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 ?
[1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java#L437
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363631613
##########
File path: 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);
Review comment:
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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363631710
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
##########
@@ -177,6 +177,38 @@
+ "ON \"t\".\"DEPTNO\" = \"t0\".\"DEPTNO\"");
}
+ @Test public void testGroupByWithLimitPlan() {
+ CalciteAssert.model(JdbcTest.SCOTT_MODEL)
+ .query("select sum(sal)\n"
+ + "from scott.emp \n"
+ + "group by ename limit 10")
+ .planHasSql("SELECT SUM(\"SAL\")\n"
Review comment:
You should add the tests in `RelToSqlConverterTest`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363630992
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -227,8 +227,13 @@ public Result visit(Project e) {
if (isStar(e.getChildExps(), e.getInput().getRowType(), e.getRowType())) {
return x;
}
- final Builder builder =
- x.builder(e, Clause.SELECT);
+ final Builder builder;
+ if (e.getInput() instanceof Sort) {
+ builder = x.builder(e);
+ builder.clauses.add(Clause.SELECT);
+ } else {
Review comment:
`builder.clauses.add(Clause.SELECT)` is useless.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363573662
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
##########
@@ -177,6 +177,38 @@
+ "ON \"t\".\"DEPTNO\" = \"t0\".\"DEPTNO\"");
}
+ @Test public void testGroupByWithLimitPlan() {
+ CalciteAssert.model(JdbcTest.SCOTT_MODEL)
+ .query("select sum(sal)\n"
+ + "from scott.emp \n"
+ + "group by ename limit 10")
+ .planHasSql("SELECT SUM(\"SAL\")\n"
+ + "FROM \"SCOTT\".\"EMP\"\n"
+ + "GROUP BY \"ENAME\"\n"
+ + "LIMIT 10");
+ }
+
+ @Test public void testSelectImplicitAliasByStar() {
+ CalciteAssert.model(JdbcTest.FOODMART_MODEL)
+ .query(
+ "select \"product_id\" + 1, * from ("
+ + "select * from\n"
+ + "("
+ + "select \"product_id\", sum(\"store_sales\") from \"sales_fact_1997\" group by \"product_id\") A\n"
+ + "union all\n"
+ + "select * from (\n"
+ + "select \"product_id\", sum(\"store_sales\") from \"sales_fact_1997\" group by \"product_id\") B\n"
+ + ") C")
+ .planHasSql("SELECT \"product_id\" + 1, \"product_id\", \"EXPR$1\"\n" +
+ "FROM (SELECT \"product_id\", COALESCE(SUM(\"store_sales\"), 0) AS \"EXPR$1\"\n" +
+ "FROM \"foodmart\".\"sales_fact_1997\"\n" +
+ "GROUP BY \"product_id\"\n" +
+ "UNION ALL\n" +
+ "SELECT \"product_id\", COALESCE(SUM(\"store_sales\"), 0) AS \"EXPR$1\"\n" +
+ "FROM \"foodmart\".\"sales_fact_1997\"\n" +
+ "GROUP BY \"product_id\") AS \"t1\"");
+ }
Review comment:
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"
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r364653745
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -227,8 +227,13 @@ public Result visit(Project e) {
if (isStar(e.getChildExps(), e.getInput().getRowType(), e.getRowType())) {
return x;
}
- final Builder builder =
- x.builder(e, Clause.SELECT);
+ final Builder builder;
+ if (e.getInput() instanceof Sort) {
+ builder = x.builder(e);
+ builder.clauses.add(Clause.SELECT);
+ } else {
Review comment:
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 removed 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);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on issue #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on issue #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#issuecomment-572481001
> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363601885
##########
File path: 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);
+ } else {
+ ordinalMap.put(lowerName, node);
+ }
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#issuecomment-572449543
Can you fix the code conflict ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r362508396
##########
File path: piglet/src/test/java/org/apache/calcite/test/PigRelOpTest.java
##########
@@ -468,14 +468,14 @@ private Fluent pig(String script) {
+ "HIREDATE, SAL, COMM, DEPTNO)) AS A\n"
+ " FROM scott.EMP\n"
+ " GROUP BY DEPTNO) AS $cor4,\n"
- + " LATERAL (SELECT COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
- + " FROM (SELECT ENAME, JOB, DEPTNO, SAL\n"
+ + " LATERAL (SELECT X\n"
+ + " FROM (SELECT 'all' AS $f0, COLLECT(ROW(ENAME, JOB, DEPTNO, SAL)) AS X\n"
+ " FROM UNNEST (SELECT $cor4.A AS $f0\n"
+ " FROM (VALUES (0)) AS t (ZERO)) "
+ "AS t2 (EMPNO, ENAME, JOB, MGR, HIREDATE, SAL, COMM, DEPTNO)\n"
+ " WHERE JOB <> 'CLERK'\n"
- + " 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"
Review comment:
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](https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java#L289)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363250728
##########
File path: 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);
+ } else {
+ ordinalMap.put(lowerName, node);
+ }
Review comment:
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()
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] helloppx opened a new pull request #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
helloppx opened a new pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715
[https://issues.apache.org/jira/browse/CALCITE-3662](https://issues.apache.org/jira/browse/CALCITE-3662)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] yanlin-Lynn commented on issue #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on issue #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#issuecomment-572465553
We can reproduce the case in `RelToSqlConverterTest`, you can apply `SortProjectTransposeRule.INSTANCE` rule to make sure Sort transpose Project
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363300931
##########
File path: 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);
+ } else {
+ ordinalMap.put(lowerName, node);
+ }
Review comment:
Hi @helloppx ~
Thanks a lot for digging into this ~
Are you sure all the failed tests still generate the correct sql by semantics ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r363302696
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
##########
@@ -177,6 +177,38 @@
+ "ON \"t\".\"DEPTNO\" = \"t0\".\"DEPTNO\"");
}
+ @Test public void testGroupByWithLimitPlan() {
+ CalciteAssert.model(JdbcTest.SCOTT_MODEL)
+ .query("select sum(sal)\n"
+ + "from scott.emp \n"
+ + "group by ename limit 10")
+ .planHasSql("SELECT SUM(\"SAL\")\n"
+ + "FROM \"SCOTT\".\"EMP\"\n"
+ + "GROUP BY \"ENAME\"\n"
+ + "LIMIT 10");
+ }
+
+ @Test public void testSelectImplicitAliasByStar() {
+ CalciteAssert.model(JdbcTest.FOODMART_MODEL)
+ .query(
+ "select \"product_id\" + 1, * from ("
+ + "select * from\n"
+ + "("
+ + "select \"product_id\", sum(\"store_sales\") from \"sales_fact_1997\" group by \"product_id\") A\n"
+ + "union all\n"
+ + "select * from (\n"
+ + "select \"product_id\", sum(\"store_sales\") from \"sales_fact_1997\" group by \"product_id\") B\n"
+ + ") C")
+ .planHasSql("SELECT \"product_id\" + 1, \"product_id\", \"EXPR$1\"\n" +
+ "FROM (SELECT \"product_id\", COALESCE(SUM(\"store_sales\"), 0) AS \"EXPR$1\"\n" +
+ "FROM \"foodmart\".\"sales_fact_1997\"\n" +
+ "GROUP BY \"product_id\"\n" +
+ "UNION ALL\n" +
+ "SELECT \"product_id\", COALESCE(SUM(\"store_sales\"), 0) AS \"EXPR$1\"\n" +
+ "FROM \"foodmart\".\"sales_fact_1997\"\n" +
+ "GROUP BY \"product_id\") AS \"t1\"");
+ }
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] jinxing64 commented on a change in pull request #1715:
[CALCITE-3662] Generate wrong SQL when plan contains
Project(Sort(Aggregate)) and aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#discussion_r364073568
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -227,8 +227,13 @@ public Result visit(Project e) {
if (isStar(e.getChildExps(), e.getInput().getRowType(), e.getRowType())) {
return x;
}
- final Builder builder =
- x.builder(e, Clause.SELECT);
+ final Builder builder;
+ if (e.getInput() instanceof Sort) {
+ builder = x.builder(e);
+ builder.clauses.add(Clause.SELECT);
+ } else {
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] yanlin-Lynn commented on issue #1715: [CALCITE-3662]
Generate wrong SQL when plan contains Project(Sort(Aggregate)) and
aggregate field has no alias
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on issue #1715: [CALCITE-3662] Generate wrong SQL when plan contains Project(Sort(Aggregate)) and aggregate field has no alias
URL: https://github.com/apache/calcite/pull/1715#issuecomment-571870272
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");
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services