You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "mihaibudiu (via GitHub)" <gi...@apache.org> on 2024/12/21 01:11:32 UTC
[PR] [CALCITE-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
mihaibudiu opened a new pull request, #4107:
URL: https://github.com/apache/calcite/pull/4107
(no comment)
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "ILuffZhe (via GitHub)" <gi...@apache.org>.
ILuffZhe commented on code in PR #4107:
URL: https://github.com/apache/calcite/pull/4107#discussion_r1897004931
##########
core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml:
##########
@@ -7654,6 +7654,28 @@ from orders]]>
LogicalDelta
LogicalProject(ROWTIME=[$0], PRODUCTID=[$1], ORDERID=[$2], C=[COUNT() OVER (PARTITION BY $1 ORDER BY $0 RANGE 1000:INTERVAL SECOND PRECEDING)])
LogicalTableScan(table=[[CATALOG, SALES, ORDERS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testStructCast">
+ <Resource name="sql">
+ <![CDATA[select ROW(1, 'x') = ROW('x', 1)]]>
Review Comment:
Got it. Thanks for the illustration.
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "ILuffZhe (via GitHub)" <gi...@apache.org>.
ILuffZhe commented on code in PR #4107:
URL: https://github.com/apache/calcite/pull/4107#discussion_r1896448982
##########
core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml:
##########
@@ -7654,6 +7654,28 @@ from orders]]>
LogicalDelta
LogicalProject(ROWTIME=[$0], PRODUCTID=[$1], ORDERID=[$2], C=[COUNT() OVER (PARTITION BY $1 ORDER BY $0 RANGE 1000:INTERVAL SECOND PRECEDING)])
LogicalTableScan(table=[[CATALOG, SALES, ORDERS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testStructCast">
+ <Resource name="sql">
+ <![CDATA[select ROW(1, 'x') = ROW('x', 1)]]>
+ </Resource>
+ <Resource name="plan">
+ <![CDATA[
+LogicalProject(EXPR$0=[=(1, CAST('x'):INTEGER NOT NULL)])
+ LogicalValues(tuples=[[{ 0 }]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testStructCast1">
+ <Resource name="sql">
+ <![CDATA[select CAST(CAST(ROW('x', 1) AS ROW(l INTEGER, r DOUBLE)) AS ROW(l BIGINT, r INTEGER)) = ROW(RAND(), RAND())]]>
+ </Resource>
+ <Resource name="plan">
+ <![CDATA[
+LogicalProject(EXPR$0=[AND(=(CAST(CAST('x'):INTEGER NOT NULL):DOUBLE NOT NULL, RAND()), =(1.0E0, RAND()))])
Review Comment:
`1.0E0` comes from `cast 1 as DOUBLE`?
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "ILuffZhe (via GitHub)" <gi...@apache.org>.
ILuffZhe commented on code in PR #4107:
URL: https://github.com/apache/calcite/pull/4107#discussion_r1896423814
##########
core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml:
##########
@@ -7654,6 +7654,28 @@ from orders]]>
LogicalDelta
LogicalProject(ROWTIME=[$0], PRODUCTID=[$1], ORDERID=[$2], C=[COUNT() OVER (PARTITION BY $1 ORDER BY $0 RANGE 1000:INTERVAL SECOND PRECEDING)])
LogicalTableScan(table=[[CATALOG, SALES, ORDERS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testStructCast">
+ <Resource name="sql">
+ <![CDATA[select ROW(1, 'x') = ROW('x', 1)]]>
Review Comment:
I'm not sure I under this correctly. I think `select ROW(1, 'x') = ROW('y', 1)` may explain your fix better.
Like you mentioned above:
'CAST(ROW(x0, x1) AS (t0, t1)) = ROW(y0, y1)' becomes 'CAST(x0 as t0) = y0 AND CAST(x1 as t1) = y1'
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4107:
URL: https://github.com/apache/calcite/pull/4107#discussion_r1894526065
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -563,9 +563,15 @@ private Expression getConvertExpression(
case TINYINT:
case SMALLINT: {
if (SqlTypeName.NUMERIC_TYPES.contains(sourceType.getSqlTypeName())) {
+ Type javaClass = typeFactory.getJavaClass(targetType);
Review Comment:
This change is only indirectly related to this PR: without this change, the code generated for ROW casts crashes at runtime.
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4107:
URL: https://github.com/apache/calcite/pull/4107#issuecomment-2557968188
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4107) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4107&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4107&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4107&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [96.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4107&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4107&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4107)
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4107:
URL: https://github.com/apache/calcite/pull/4107#issuecomment-2561379425
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4107) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4107&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4107&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4107&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [96.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4107&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4107&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4107)
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #4107:
URL: https://github.com/apache/calcite/pull/4107#discussion_r1894653027
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -563,9 +563,15 @@ private Expression getConvertExpression(
case TINYINT:
case SMALLINT: {
if (SqlTypeName.NUMERIC_TYPES.contains(sourceType.getSqlTypeName())) {
+ Type javaClass = typeFactory.getJavaClass(targetType);
Review Comment:
Why does it crash? It feels a bit strange
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4107:
URL: https://github.com/apache/calcite/pull/4107#discussion_r1896896439
##########
core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml:
##########
@@ -7654,6 +7654,28 @@ from orders]]>
LogicalDelta
LogicalProject(ROWTIME=[$0], PRODUCTID=[$1], ORDERID=[$2], C=[COUNT() OVER (PARTITION BY $1 ORDER BY $0 RANGE 1000:INTERVAL SECOND PRECEDING)])
LogicalTableScan(table=[[CATALOG, SALES, ORDERS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testStructCast">
+ <Resource name="sql">
+ <![CDATA[select ROW(1, 'x') = ROW('x', 1)]]>
Review Comment:
This becomes `CAST('x' AS INTEGER) = 1 AND 1 = CAST('x' AS INTEGER)`, which is then simplified
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4107:
URL: https://github.com/apache/calcite/pull/4107#discussion_r1896940004
##########
core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml:
##########
@@ -7654,6 +7654,28 @@ from orders]]>
LogicalDelta
LogicalProject(ROWTIME=[$0], PRODUCTID=[$1], ORDERID=[$2], C=[COUNT() OVER (PARTITION BY $1 ORDER BY $0 RANGE 1000:INTERVAL SECOND PRECEDING)])
LogicalTableScan(table=[[CATALOG, SALES, ORDERS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testStructCast">
+ <Resource name="sql">
+ <![CDATA[select ROW(1, 'x') = ROW('x', 1)]]>
Review Comment:
I will change the test the way you suggested
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4107:
URL: https://github.com/apache/calcite/pull/4107#issuecomment-2560439169
This PR fixes a real bug and should not be too hard to review.
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #4107:
URL: https://github.com/apache/calcite/pull/4107
--
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-6742] StandardConvertletTable.convertCall loses casts from ROW comparisons [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4107:
URL: https://github.com/apache/calcite/pull/4107#discussion_r1896896843
##########
core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml:
##########
@@ -7654,6 +7654,28 @@ from orders]]>
LogicalDelta
LogicalProject(ROWTIME=[$0], PRODUCTID=[$1], ORDERID=[$2], C=[COUNT() OVER (PARTITION BY $1 ORDER BY $0 RANGE 1000:INTERVAL SECOND PRECEDING)])
LogicalTableScan(table=[[CATALOG, SALES, ORDERS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testStructCast">
+ <Resource name="sql">
+ <![CDATA[select ROW(1, 'x') = ROW('x', 1)]]>
+ </Resource>
+ <Resource name="plan">
+ <![CDATA[
+LogicalProject(EXPR$0=[=(1, CAST('x'):INTEGER NOT NULL)])
+ LogicalValues(tuples=[[{ 0 }]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testStructCast1">
+ <Resource name="sql">
+ <![CDATA[select CAST(CAST(ROW('x', 1) AS ROW(l INTEGER, r DOUBLE)) AS ROW(l BIGINT, r INTEGER)) = ROW(RAND(), RAND())]]>
+ </Resource>
+ <Resource name="plan">
+ <![CDATA[
+LogicalProject(EXPR$0=[AND(=(CAST(CAST('x'):INTEGER NOT NULL):DOUBLE NOT NULL, RAND()), =(1.0E0, RAND()))])
Review Comment:
yes, there are many places where Calcite simplifies expressions, it's hard to write a test exhibits just the change I made.
--
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