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 2025/03/25 21:57:39 UTC

[PR] [CALCITE-6911] SqlItemOperator.inferReturnType throws AssertionError for out of bounds accesses [calcite]

mihaibudiu opened a new pull request, #4264:
URL: https://github.com/apache/calcite/pull/4264

   (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-6911] SqlItemOperator.inferReturnType throws AssertionError for out of bounds accesses [calcite]

Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4264:
URL: https://github.com/apache/calcite/pull/4264#discussion_r2019646240


##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13289,10 +13289,9 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
     f.check("select \"T\".\"X\"[2] "
             + "from (VALUES (ROW(ROW(3, CAST(NULL AS INTEGER)), ROW(4, 8)))) as T(x, y)",
         SqlTests.ANY_TYPE_CHECKER, isNullValue());
-    f.checkFails("select \"T\".\"X\"[1 + CAST(NULL AS INTEGER)] "
+    f.checkFails("select ^\"T\".\"X\"[1 + CAST(NULL AS INTEGER)]^ "

Review Comment:
   Can we be more precise here, such as displaying `1+CAST (NULL AS INTEGER)` in the exception message?



-- 
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-6911] SqlItemOperator.inferReturnType throws AssertionError for out of bounds accesses [calcite]

Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4264:
URL: https://github.com/apache/calcite/pull/4264#discussion_r2019646652


##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13289,10 +13289,9 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
     f.check("select \"T\".\"X\"[2] "
             + "from (VALUES (ROW(ROW(3, CAST(NULL AS INTEGER)), ROW(4, 8)))) as T(x, y)",
         SqlTests.ANY_TYPE_CHECKER, isNullValue());
-    f.checkFails("select \"T\".\"X\"[1 + CAST(NULL AS INTEGER)] "
+    f.checkFails("select ^\"T\".\"X\"[1 + CAST(NULL AS INTEGER)]^ "

Review Comment:
   The position is included, that's what ^ stands for in these tests. The position is between the two carets. 
   



-- 
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-6911] SqlItemOperator.inferReturnType throws AssertionError for out of bounds accesses [calcite]

Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4264:
URL: https://github.com/apache/calcite/pull/4264#discussion_r2019646240


##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13289,10 +13289,9 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
     f.check("select \"T\".\"X\"[2] "
             + "from (VALUES (ROW(ROW(3, CAST(NULL AS INTEGER)), ROW(4, 8)))) as T(x, y)",
         SqlTests.ANY_TYPE_CHECKER, isNullValue());
-    f.checkFails("select \"T\".\"X\"[1 + CAST(NULL AS INTEGER)] "
+    f.checkFails("select ^\"T\".\"X\"[1 + CAST(NULL AS INTEGER)]^ "

Review Comment:
   Can we be more precise here, such as displaying `1+CAST (NULL AS INTEGER)` or display the position in the exception message?



-- 
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-6911] SqlItemOperator.inferReturnType throws AssertionError for out of bounds accesses [calcite]

Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #4264:
URL: https://github.com/apache/calcite/pull/4264


-- 
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-6911] SqlItemOperator.inferReturnType throws AssertionError for out of bounds accesses [calcite]

Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4264:
URL: https://github.com/apache/calcite/pull/4264#issuecomment-2756026013

   To make it easier to review, please note that the new code in SqlItemOperator is essentially lifted from the existing code from SqlItemOperator.inferReturnType; it is just that these checks should be done earlier, and should not throw AssertionError, but report a normal error.


-- 
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-6911] SqlItemOperator.inferReturnType throws AssertionError for out of bounds accesses [calcite]

Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4264:
URL: https://github.com/apache/calcite/pull/4264#discussion_r2019646240


##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13289,10 +13289,9 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
     f.check("select \"T\".\"X\"[2] "
             + "from (VALUES (ROW(ROW(3, CAST(NULL AS INTEGER)), ROW(4, 8)))) as T(x, y)",
         SqlTests.ANY_TYPE_CHECKER, isNullValue());
-    f.checkFails("select \"T\".\"X\"[1 + CAST(NULL AS INTEGER)] "
+    f.checkFails("select ^\"T\".\"X\"[1 + CAST(NULL AS INTEGER)]^ "

Review Comment:
   Can we be more precise here, such as displaying `1+CAST (NULL AS INTEGER)` or display the position of the problem? in the exception message?



-- 
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-6911] SqlItemOperator.inferReturnType throws AssertionError for out of bounds accesses [calcite]

Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4264:
URL: https://github.com/apache/calcite/pull/4264#issuecomment-2752642657

   I have kept the assertions unchanged, but added additional validation which should prevent these assertions from ever being fired. The validation has to be done in `checkOperandTypes` to provide nice validator error messages; by the time `inferReturnTypes` is reached we no longer have a handle to the required callBinding.


-- 
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