[GitHub] [calcite] Jiabao-Sun commented on a diff in pull request #3430: [CALCITE-6008] ARRAY_AGG should returns ARRAY NULL when there's no input rows

2023-09-21 Thread via GitHub 


Jiabao-Sun commented on code in PR #3430:
URL: https://github.com/apache/calcite/pull/3430#discussion_r119224


##
core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java:
##
@@ -645,6 +645,13 @@ public static SqlCall stripSeparator(SqlCall call) {
   public static final SqlReturnTypeInference TO_ARRAY =
   ARG0.andThen(SqlTypeTransforms.TO_ARRAY);
 
+  /**
+   * Same as {@link #TO_ARRAY} but returns with nullability if any of the
+   * operands is nullable.
+   */
+  public static final SqlReturnTypeInference ARRAY_NULLABLE =

Review Comment:
   Thanks @tanclary for the review.
   `ARRAY_NULLABLE`'s naming refers to `MULTISET` because they are all 
collection types.
   I am not sure if this is appropriate. 
   If you think it should be named `TO_ARRAY_NULLABLE `, I can correct it.
   
   ```java
 /**
  * Returns a MULTISET type.
  *
  * For example, given INTEGER, returns
  * INTEGER MULTISET.
  */
 public static final SqlReturnTypeInference TO_MULTISET =
 ARG0.andThen(SqlTypeTransforms.TO_MULTISET);
   
 /**
  * Same as {@link #MULTISET} but returns with nullability if any of the
  * operands is nullable.
  */
 public static final SqlReturnTypeInference MULTISET_NULLABLE =
 MULTISET.andThen(SqlTypeTransforms.TO_NULLABLE);
   ```



-- 
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-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] Jiabao-Sun commented on a diff in pull request #3430: [CALCITE-6008] ARRAY_AGG should returns ARRAY NULL when there's no input rows

2023-09-16 Thread via GitHub 


Jiabao-Sun commented on code in PR #3430:
URL: https://github.com/apache/calcite/pull/3430#discussion_r1327999465


##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -9314,11 +9314,12 @@ private static void 
checkGroupConcatFuncFails(SqlOperatorFixture t) {
   }
 
   private static void checkArrayAggFunc(SqlOperatorFixture f) {
-f.setFor(SqlLibraryOperators.ARRAY_CONCAT_AGG, VM_FENNEL, VM_JAVA);
+f.setFor(SqlLibraryOperators.ARRAY_AGG, VM_FENNEL, VM_JAVA);

Review Comment:
   But I can't find any proper rules to verify that `ARRAY_AGG($0)` should 
returns  `ARRAY NULL` type.



-- 
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-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] Jiabao-Sun commented on a diff in pull request #3430: [CALCITE-6008] ARRAY_AGG should returns ARRAY NULL when there's no input rows

2023-09-16 Thread via GitHub 


Jiabao-Sun commented on code in PR #3430:
URL: https://github.com/apache/calcite/pull/3430#discussion_r1327990074


##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -9314,11 +9314,12 @@ private static void 
checkGroupConcatFuncFails(SqlOperatorFixture t) {
   }
 
   private static void checkArrayAggFunc(SqlOperatorFixture f) {
-f.setFor(SqlLibraryOperators.ARRAY_CONCAT_AGG, VM_FENNEL, VM_JAVA);
+f.setFor(SqlLibraryOperators.ARRAY_AGG, VM_FENNEL, VM_JAVA);

Review Comment:
   Sorry @mihaibudiu, I read the example again and it makes sense to me.
   I can add some test in `RelOptRulesTest` as 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.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] Jiabao-Sun commented on a diff in pull request #3430: [CALCITE-6008] ARRAY_AGG should returns ARRAY NULL when there's no input rows

2023-09-16 Thread via GitHub 


Jiabao-Sun commented on code in PR #3430:
URL: https://github.com/apache/calcite/pull/3430#discussion_r1327989243


##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -9314,11 +9314,12 @@ private static void 
checkGroupConcatFuncFails(SqlOperatorFixture t) {
   }
 
   private static void checkArrayAggFunc(SqlOperatorFixture f) {
-f.setFor(SqlLibraryOperators.ARRAY_CONCAT_AGG, VM_FENNEL, VM_JAVA);
+f.setFor(SqlLibraryOperators.ARRAY_AGG, VM_FENNEL, VM_JAVA);

Review Comment:
   Thanks @mihaibudiu.
   The main issue with this question is incorrect function return type 
inference, and it seem's not be related to the planner rules. And the old test 
case of this line uses the wrong operator.
   
   



-- 
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-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org