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 2019/12/20 13:02:23 UTC
[GitHub] [calcite] helloppx opened a new pull request #1680: [CALCITE-3621]
Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
helloppx opened a new pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680
[https://issues.apache.org/jira/browse/CALCITE-3621#](url)
Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
: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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360756860
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
When JdbcSort is used, a regression issue will be trigged: JdbcTest#testSelfJoinDifferentColumns()
RelToSqlConverter visits the Sort and then produce a SQL "SELECT * from (t JOIN t0 ...) order by ...)". However t and t0 include one same name field "last_name". The outter Project will miss the correct field name.
So I think this change is needed.
Before SQL:
select e1."full_name"
from "foodmart"."employee" as e1
join "foodmart"."employee" as e2 on e1."first_name" = e2."last_name"
order by e1."last_name" limit 3
REL:
JdbcToEnumerableConverter
JdbcProject(full_name=[$1], last_name=[$3])
JdbcSort(sort0=[$3], dir0=[ASC], fetch=[3])
JdbcJoin(condition=[=($2, $0)], joinType=[inner])
JdbcProject(last_name=[$3])
JdbcTableScan(table=[[foodmart, employee]])
JdbcProject(full_name=[$1], first_name=[$2], last_name=[$3])
JdbcTableScan(table=[[foodmart, employee]])
After SQL:
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT *
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360749347
##########
File path: core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
##########
@@ -765,10 +766,11 @@ public RelNode convert(RelNode rel) {
*/
public RelNode convert(Sort sort, boolean convertInputTraits) {
final RelTraitSet traitSet = sort.getTraitSet().replace(out);
+ RelTraitSet inputTraitSet = traitSet.replace(RelCollations.EMPTY);
Review comment:
Yes, your change is better :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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360756860
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
When JdbcSort is used, a regression issue will be trigged: JdbcTest#testSelfJoinDifferentColumns()
RelToSqlConverter visit this Sort and then produce a SQL "SELECT * from (t JOIN t0 ...) order by ...)". However t and t0 include one same name field "last_name"
Before SQL:
select e1."full_name"
from "foodmart"."employee" as e1
join "foodmart"."employee" as e2 on e1."first_name" = e2."last_name"
order by e1."last_name" limit 3
REL:
JdbcToEnumerableConverter
JdbcProject(full_name=[$1], last_name=[$3])
JdbcSort(sort0=[$3], dir0=[ASC], fetch=[3])
JdbcJoin(condition=[=($2, $0)], joinType=[inner])
JdbcProject(last_name=[$3])
JdbcTableScan(table=[[foodmart, employee]])
JdbcProject(full_name=[$1], first_name=[$2], last_name=[$3])
JdbcTableScan(table=[[foodmart, employee]])
After SQL:
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT *
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r361313374
##########
File path: core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
##########
@@ -765,10 +765,11 @@ public RelNode convert(RelNode rel) {
*/
public RelNode convert(Sort sort, boolean convertInputTraits) {
final RelTraitSet traitSet = sort.getTraitSet().replace(out);
+ final RelTraitSet inputTraitSet = sort.getInput().getTraitSet().replace(out);
Review comment:
Move this line below `if (convertInputTraits)` where it is actually used.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360819949
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
This change is for `JdbcTest#testSelfJoinDifferentColumns()` mainly, `testMatchRecognizePatternExpression12` is affected by the side effects, so I have to change it
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360756860
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
When JdbcSort is used, a regression issue will be trigged: JdbcTest#testSelfJoinDifferentColumns()
RelToSqlConverter visits the Sort and then produce a SQL "SELECT * from (t JOIN t0 ...) order by ...)". However t and t0 include a field that has same name "last_name". The outter Project will miss the correct field name.
So I think this change is needed.
Following is the regression issue info:
Before SQL:
select e1."full_name"
from "foodmart"."employee" as e1
join "foodmart"."employee" as e2 on e1."first_name" = e2."last_name"
order by e1."last_name" limit 3
REL:
JdbcToEnumerableConverter
--JdbcProject(full_name=[$1], last_name=[$3])
----JdbcSort(sort0=[$3], dir0=[ASC], fetch=[3])
------JdbcJoin(condition=[=($2, $0)], joinType=[inner])
--------JdbcProject(last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
--------JdbcProject(full_name=[$1], first_name=[$2], last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
After SQL:
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT *
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360714235
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
Why this change is necessary ?
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360714260
##########
File path: core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
##########
@@ -765,10 +766,11 @@ public RelNode convert(RelNode rel) {
*/
public RelNode convert(Sort sort, boolean convertInputTraits) {
final RelTraitSet traitSet = sort.getTraitSet().replace(out);
+ RelTraitSet inputTraitSet = traitSet.replace(RelCollations.EMPTY);
Review comment:
Why it's not as below
```
final RelTraitSet inputTraitSet = sort.getInput().getTraitSet().replace(out);
```
----------------------------------------------------------------
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 #1680: [CALCITE-3621] Push
down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
helloppx commented on issue #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#issuecomment-569859565
> The commit message is a little confusing. IMO, it is better to use the title of the jira, namely `JDBC adapter can't push down sort to DB` as commit message.
Thank you @chunweilei , I have updated it.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360819949
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
This change is mainly for `JdbcTest#testSelfJoinDifferentColumns()` , `testMatchRecognizePatternExpression12` is affected by the side effects, so I have to change it
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r361402419
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
Hi Danny ~
I gave the suggestion to keep `select *` when `Sort` is the outmost rel, which I think is much simpler.
I'm ok if you think that Calcite should expand the select list for `Sort`.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r360756860
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
When JdbcSort is used, a regression issue will be trigged: JdbcTest#testSelfJoinDifferentColumns()
Following is the regression issue info:
Input SQL: A
select e1."full_name"
from "foodmart"."employee" as e1
join "foodmart"."employee" as e2 on e1."first_name" = e2."last_name"
order by e1."last_name" limit 3
----
Best REL:
JdbcToEnumerableConverter
--JdbcProject(full_name=[$1], last_name=[$3])
----JdbcSort(sort0=[$3], dir0=[ASC], fetch=[3])
------JdbcJoin(condition=[=($2, $0)], joinType=[inner])
--------JdbcProject(last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
--------JdbcProject(full_name=[$1], first_name=[$2], last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
----
Output SQL: B
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT *
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
----
After fix, output SQL: C
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT "t"."last_name", "t0"."full_name", "t0"."first_name", "t0"."last_name" AS "last_name0"
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
----
SQL B is wrong, since "t2" is a subquery but it's select list is ambiguous. Database complains "last_name0" cannot be found in schema.
that is, the SQL of RelToSqlConverter#visit(Sort e) generated should has explicit select list when the Sort is not outter rel.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r361627381
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
Thanks @jinxing64 , after some research i agree that maybe fixing the sort project list is the easiest way to go, but i would suggest to use `stack.size() == 1` to decide if this sort is a root node instead of `stack.getLast().r != e`.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r361360850
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
I have updated the description of regission issue again, please help review :D
`if (stack.getLast().r != e)` can make a decision whether currently Sort rel is the outermost rel.
If it is, the select list of generated SQL can be "select *", It is more simplier.
If not, the select list of generated SQL shoud contain explicit column name.
Yes, `CalcitePrepareImpl#implement(RelRoot root)` generate SQL by `root.rel`，but the final select field be controled by `root.fields`.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r361313332
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
Just curious, shouldn't we generate `SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"` instead of `select *` ?
Why we need a decision `if (stack.getLast().r != e)` it looks weird.
Also, the generated sql outputs one more column than the original, i think it is because we generate the sql using the RelRoot.rel directly. We better fix this.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360756860
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
When JdbcSort is used, a regression issue will be trigged: JdbcTest#testSelfJoinDifferentColumns()
RelToSqlConverter visits the Sort and then produce a SQL "SELECT * from (t JOIN t0 ...) order by ...)". However t and t0 include one same name field "last_name". The outter Project will miss the correct field name.
So I think this change is needed.
Before SQL:
select e1."full_name"
from "foodmart"."employee" as e1
join "foodmart"."employee" as e2 on e1."first_name" = e2."last_name"
order by e1."last_name" limit 3
Following is the regression issue info:
REL:
JdbcToEnumerableConverter
--JdbcProject(full_name=[$1], last_name=[$3])
----JdbcSort(sort0=[$3], dir0=[ASC], fetch=[3])
------JdbcJoin(condition=[=($2, $0)], joinType=[inner])
--------JdbcProject(last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
--------JdbcProject(full_name=[$1], first_name=[$2], last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
After SQL:
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT *
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
----------------------------------------------------------------
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 closed pull request #1680: [CALCITE-3621] Push
down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
danny0405 closed pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360874450
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
Thanks for explanation ~
Since it's very common to execute sql like `select * from XXX order by col`, shall we wrap the change within a check like below:
```
if (stack.getLast().r != e) {
...
}
```
Thus we can save the change in`RelToSqlConverterTest.java` and keep the converted Sql String simplier.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360756860
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
When JdbcSort is used, a regression issue will be trigged: JdbcTest#testSelfJoinDifferentColumns()
RelToSqlConverter visits the Sort and then produce a SQL "SELECT * from (t JOIN t0 ...) order by ...)". However t and t0 include one same name field "last_name". The outter Project will miss the correct field name.
So I think this change is needed.
Following is the regression issue info:
Before SQL:
select e1."full_name"
from "foodmart"."employee" as e1
join "foodmart"."employee" as e2 on e1."first_name" = e2."last_name"
order by e1."last_name" limit 3
REL:
JdbcToEnumerableConverter
--JdbcProject(full_name=[$1], last_name=[$3])
----JdbcSort(sort0=[$3], dir0=[ASC], fetch=[3])
------JdbcJoin(condition=[=($2, $0)], joinType=[inner])
--------JdbcProject(last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
--------JdbcProject(full_name=[$1], first_name=[$2], last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
After SQL:
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT *
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r361360850
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
I have updated the description of regission issue again, please help review :D
`if (stack.getLast().r != e)` can make a decision whether currently Sort rel is the outermost rel.
If it is, the select list of generated SQL can be "select *", It is more simplier.
If not, the select list of generated SQL shoud contain explicit column name.
Yes, `CalcitePrepareImpl#implement(RelRoot root)` generate SQL by `root.rel`，but the correctly select field be controled by `root.fields`.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r360756860
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
When JdbcSort is used, a regression issue will be trigged: JdbcTest#testSelfJoinDifferentColumns()
Following is the regression issue info:
Input SQL: A
select e1."full_name"
from "foodmart"."employee" as e1
join "foodmart"."employee" as e2 on e1."first_name" = e2."last_name"
order by e1."last_name" limit 3
----
Best REL:
JdbcToEnumerableConverter
--JdbcProject(full_name=[$1], last_name=[$3])
----JdbcSort(sort0=[$3], dir0=[ASC], fetch=[3])
------JdbcJoin(condition=[=($2, $0)], joinType=[inner])
--------JdbcProject(last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
--------JdbcProject(full_name=[$1], first_name=[$2], last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
----
Output SQL: B
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT *
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
----
After fix, output SQL: C
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT "t"."last_name", "t0"."full_name", "t0"."first_name", "t0"."last_name" AS "last_name0"
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
----
SQL B is wrong, since "t2" is a subquery but it's select list is ambiguous. Database complains "last_name0" cannot be found in schema.
that is, the SQL of RelToSqlConverter#visit(Sort e) generated should has explicit select list when the Sort is not the outermost rel.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r361054009
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
Thank you so much for your help ~ :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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r360756860
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
When JdbcSort is used, a regression issue will be trigged: JdbcTest#testSelfJoinDifferentColumns()
Following is the regression issue info:
Input SQL: A
select e1."full_name"
from "foodmart"."employee" as e1
join "foodmart"."employee" as e2 on e1."first_name" = e2."last_name"
order by e1."last_name" limit 3
Best REL:
JdbcToEnumerableConverter
--JdbcProject(full_name=[$1], last_name=[$3])
----JdbcSort(sort0=[$3], dir0=[ASC], fetch=[3])
------JdbcJoin(condition=[=($2, $0)], joinType=[inner])
--------JdbcProject(last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
--------JdbcProject(full_name=[$1], first_name=[$2], last_name=[$3])
----------JdbcTableScan(table=[[foodmart, employee]])
Output SQL: B
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT *
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
After fix, output SQL: C
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT "t"."last_name", "t0"."full_name", "t0"."first_name", "t0"."last_name" AS "last_name0"
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
SQL B is wrong, since "t2" is a subquery but it's select list is ambiguous. Database complains "last_name0" cannot be found in schema.
that is, the SQL of RelToSqlConverter#visit(Sort e) generated should has explicit select list when the Sort is not outter rel.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360795127
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
Without the change in RelToSqlConverter.java, I still passed the unchanged `testMatchRecognizePatternExpression12`
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r361889043
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
Thank you guys, I have updated it.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of *
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of *
URL: https://github.com/apache/calcite/pull/1680#discussion_r360756860
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
When JdbcSort is used, a regression issue will be trigged: JdbcTest#testSelfJoinDifferentColumns()
RelToSqlConverter visits the Sort and then produce a SQL "SELECT * from (t JOIN t0 ...) order by ...)". However t and t0 include one same name field "last_name".
So I think this change is needed.
Before SQL:
select e1."full_name"
from "foodmart"."employee" as e1
join "foodmart"."employee" as e2 on e1."first_name" = e2."last_name"
order by e1."last_name" limit 3
REL:
JdbcToEnumerableConverter
JdbcProject(full_name=[$1], last_name=[$3])
JdbcSort(sort0=[$3], dir0=[ASC], fetch=[3])
JdbcJoin(condition=[=($2, $0)], joinType=[inner])
JdbcProject(last_name=[$3])
JdbcTableScan(table=[[foodmart, employee]])
JdbcProject(full_name=[$1], first_name=[$2], last_name=[$3])
JdbcTableScan(table=[[foodmart, employee]])
After SQL:
SELECT "t2"."full_name", "t2"."last_name0" AS "last_name"
FROM (SELECT *
FROM (SELECT "last_name"
FROM "foodmart"."employee") AS "t"
INNER JOIN (SELECT "full_name", "first_name", "last_name"
FROM "foodmart"."employee") AS "t0" ON "t"."last_name" = "t0"."first_name"
ORDER BY "t0"."last_name" NULLS LAST
LIMIT 3) AS "t2"
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r361360850
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -634,6 +634,13 @@ public Result visit(Sort e) {
}
Result x = visitChild(0, e.getInput());
Builder builder = x.builder(e, Clause.ORDER_BY);
+ if (builder.select.getSelectList() == null) {
Review comment:
I have updated the description of regission issue again, please help review :D
`if (stack.getLast().r != e)` can make a decision whether currently Sort rel is the outermost rel.
If it is, the select list of generated SQL can be "select *", It is more simplier.
If not, the select list of generated SQL shoud contain explicit column name.
`CalcitePrepareImpl#implement(RelRoot root)` generate SQL by root.rel，but the correctly select field be controled by `root.fields`.
----------------------------------------------------------------
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 #1680:
[CALCITE-3621] Push down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
helloppx commented on a change in pull request #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#discussion_r361360920
##########
File path: core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
##########
@@ -765,10 +765,11 @@ public RelNode convert(RelNode rel) {
*/
public RelNode convert(Sort sort, boolean convertInputTraits) {
final RelTraitSet traitSet = sort.getTraitSet().replace(out);
+ final RelTraitSet inputTraitSet = sort.getInput().getTraitSet().replace(out);
Review comment:
Thank you, I will update it.
----------------------------------------------------------------
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 #1680: [CALCITE-3621] Push
down sort to DB,
SQL of Sort rel contains explicit field name instead of * when sort is not
top rel
Posted by GitBox <gi...@apache.org>.
helloppx commented on issue #1680: [CALCITE-3621] Push down sort to DB, SQL of Sort rel contains explicit field name instead of * when sort is not top rel
URL: https://github.com/apache/calcite/pull/1680#issuecomment-568823430
By the way, when we switch "calcite.enable.enumerable" to "false" without this change, this case will be failed.
`java.sql.SQLException: Error while executing SQL "explain plan for select ename
from scott.emp
order by empno": There are not enough rules to produce a node with desired properties: convention=ENUMERABLE, sort=[1].
Missing conversion is JdbcTableScan[sort: [] -> [0]]
There is 1 empty subset: rel#34:Subset#0.JDBC.SCOTT.[0], the relevant part of the original plan is as follows
0:JdbcTableScan(table=[[SCOTT, EMP]])`
----------------------------------------------------------------
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