You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "YiwenWu (via GitHub)" <gi...@apache.org> on 2024/07/15 07:30:05 UTC
[PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
YiwenWu opened a new pull request, #3856:
URL: https://github.com/apache/calcite/pull/3856
```
SELECT
"a", "x"
FROM
"myDb"."myTable",
unnest("xs") as "x";
```
original obtained sql:
```
SELECT
"$cor0"."a",
"$cor0"."xs0" AS "x" -- <-- xs0 ?
FROM
(
SELECT "a", "n1"."n11"."b", "n1"."n12"."c", "n2"."d", "xs", "e" FROM "myDb"."myTable"
) AS "$cor0",
LATERAL UNNEST (
SELECT "$cor0"."xs" FROM (VALUES (0)) AS "t" ("ZERO")
) AS "t1" ("xs") AS "t10"
```
this PR fixed obtained sql:
```
SELECT
"$cor0"."a",
"t10."xs" AS "x" -- <-- xs0 ?
FROM
(
SELECT "a", "n1"."n11"."b", "n1"."n12"."c", "n2"."d", "xs", "e" FROM "myDb"."myTable"
) AS "$cor0",
LATERAL UNNEST (
SELECT "$cor0"."xs" FROM (VALUES (0)) AS "t" ("ZERO")
) AS "t1" ("xs") AS "t10"
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3856:
URL: https://github.com/apache/calcite/pull/3856#issuecomment-2239007841
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3856) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3856&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3856&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3856&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3856)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
YiwenWu commented on code in PR #3856:
URL: https://github.com/apache/calcite/pull/3856#discussion_r1684210522
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterStructsTest.java:
##########
@@ -41,11 +41,9 @@ private RelToSqlConverterTest.Sql sql(String sql) {
@Test void testNestedSchemaSelectStar() {
String query = "SELECT * FROM \"myTable\"";
- String expected = "SELECT \"a\", "
- + "ROW(ROW(\"n1\".\"n11\".\"b\"), ROW(\"n1\".\"n12\".\"c\")) AS \"n1\", "
- + "ROW(\"n2\".\"d\") AS \"n2\", "
- + "\"e\"\n"
- + "FROM \"myDb\".\"myTable\"";
+ String expected = "SELECT \"a\", ROW(ROW(\"n1\".\"n11\".\"b\"), "
Review Comment:
There are some differences, `myTable` adds a new field `xs`.
<img width="1283" alt="image" src="https://github.com/user-attachments/assets/211a31e1-ea24-4237-bfa5-72a139b51212">
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3856:
URL: https://github.com/apache/calcite/pull/3856
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
YiwenWu commented on code in PR #3856:
URL: https://github.com/apache/calcite/pull/3856#discussion_r1684216098
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -397,7 +397,7 @@ private boolean isCommaJoin(Join join) {
public Result visit(Correlate e) {
final Result leftResult =
visitInput(e, 0)
- .resetAlias(e.getCorrelVariable(), e.getRowType());
+ .resetAlias(e.getCorrelVariable(), e.getInput(0).getRowType());
Review Comment:
It may be that fewer cases are used. Correlate is similar to Join, with leftNode and rightNode, among which e.getInput(0) is equivalent to leftNode. Therefore the RowType of e.getInput(0) is a subset of Correlate.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
YiwenWu commented on code in PR #3856:
URL: https://github.com/apache/calcite/pull/3856#discussion_r1684224310
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -397,7 +397,7 @@ private boolean isCommaJoin(Join join) {
public Result visit(Correlate e) {
final Result leftResult =
visitInput(e, 0)
- .resetAlias(e.getCorrelVariable(), e.getRowType());
+ .resetAlias(e.getCorrelVariable(), e.getInput(0).getRowType());
Review Comment:
By the way, I found that currently `Correlate` Implement toSql cannot be executed in many engines. It may be because this feature was introduced by `pig` and is only adapted to the `pig` dialect.
I intend to address this going forward.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
YiwenWu commented on code in PR #3856:
URL: https://github.com/apache/calcite/pull/3856#discussion_r1684210522
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterStructsTest.java:
##########
@@ -41,11 +41,9 @@ private RelToSqlConverterTest.Sql sql(String sql) {
@Test void testNestedSchemaSelectStar() {
String query = "SELECT * FROM \"myTable\"";
- String expected = "SELECT \"a\", "
- + "ROW(ROW(\"n1\".\"n11\".\"b\"), ROW(\"n1\".\"n12\".\"c\")) AS \"n1\", "
- + "ROW(\"n2\".\"d\") AS \"n2\", "
- + "\"e\"\n"
- + "FROM \"myDb\".\"myTable\"";
+ String expected = "SELECT \"a\", ROW(ROW(\"n1\".\"n11\".\"b\"), "
Review Comment:
Yes, there is no difference, it has been rolled back.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3856:
URL: https://github.com/apache/calcite/pull/3856#issuecomment-2227935961
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3856) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3856&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3856&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3856&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3856)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3856:
URL: https://github.com/apache/calcite/pull/3856#discussion_r1678413825
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -397,7 +397,7 @@ private boolean isCommaJoin(Join join) {
public Result visit(Correlate e) {
final Result leftResult =
visitInput(e, 0)
- .resetAlias(e.getCorrelVariable(), e.getRowType());
+ .resetAlias(e.getCorrelVariable(), e.getInput(0).getRowType());
Review Comment:
this seems like a pretty big bug, I wonder how it wasn't discovered sooner
Is it because for correlated queries the type of e is often the same as the type of e.getInput(0)?
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterStructsTest.java:
##########
@@ -41,11 +41,9 @@ private RelToSqlConverterTest.Sql sql(String sql) {
@Test void testNestedSchemaSelectStar() {
String query = "SELECT * FROM \"myTable\"";
- String expected = "SELECT \"a\", "
- + "ROW(ROW(\"n1\".\"n11\".\"b\"), ROW(\"n1\".\"n12\".\"c\")) AS \"n1\", "
- + "ROW(\"n2\".\"d\") AS \"n2\", "
- + "\"e\"\n"
- + "FROM \"myDb\".\"myTable\"";
+ String expected = "SELECT \"a\", ROW(ROW(\"n1\".\"n11\".\"b\"), "
Review Comment:
In fact, is there any difference here?
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterStructsTest.java:
##########
@@ -41,11 +41,9 @@ private RelToSqlConverterTest.Sql sql(String sql) {
@Test void testNestedSchemaSelectStar() {
String query = "SELECT * FROM \"myTable\"";
- String expected = "SELECT \"a\", "
- + "ROW(ROW(\"n1\".\"n11\".\"b\"), ROW(\"n1\".\"n12\".\"c\")) AS \"n1\", "
- + "ROW(\"n2\".\"d\") AS \"n2\", "
- + "\"e\"\n"
- + "FROM \"myDb\".\"myTable\"";
+ String expected = "SELECT \"a\", ROW(ROW(\"n1\".\"n11\".\"b\"), "
Review Comment:
I would refrain for reformatting the string to make the comparison easier
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
YiwenWu commented on code in PR #3856:
URL: https://github.com/apache/calcite/pull/3856#discussion_r1684210522
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterStructsTest.java:
##########
@@ -41,11 +41,9 @@ private RelToSqlConverterTest.Sql sql(String sql) {
@Test void testNestedSchemaSelectStar() {
String query = "SELECT * FROM \"myTable\"";
- String expected = "SELECT \"a\", "
- + "ROW(ROW(\"n1\".\"n11\".\"b\"), ROW(\"n1\".\"n12\".\"c\")) AS \"n1\", "
- + "ROW(\"n2\".\"d\") AS \"n2\", "
- + "\"e\"\n"
- + "FROM \"myDb\".\"myTable\"";
+ String expected = "SELECT \"a\", ROW(ROW(\"n1\".\"n11\".\"b\"), "
Review Comment:
String format has been rolled back. There are some differences, `myTable` adds a new field `xs`.
<img width="1283" alt="image" src="https://github.com/user-attachments/assets/211a31e1-ea24-4237-bfa5-72a139b51212">
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6218] RelToSqlConverter fails to convert correlated lateral … [calcite]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
YiwenWu commented on code in PR #3856:
URL: https://github.com/apache/calcite/pull/3856#discussion_r1684216098
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -397,7 +397,7 @@ private boolean isCommaJoin(Join join) {
public Result visit(Correlate e) {
final Result leftResult =
visitInput(e, 0)
- .resetAlias(e.getCorrelVariable(), e.getRowType());
+ .resetAlias(e.getCorrelVariable(), e.getInput(0).getRowType());
Review Comment:
It may be that fewer cases are used. Correlate is similar to Join, with leftNode and rightNode, among which e.getInput(0) is equivalent to leftNode. Therefore the `RowType` of e.getInput(0) is a subset of Correlate.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org