Handle code-reviews

chucheng92 · Feb 29, 2024 · 9956ab3 · 9956ab3
1 parent ea8c3ca
commit 9956ab3
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 22 deletions.
diff --git a/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java b/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java
@@ -1240,9 +1240,7 @@ private static RelDataType arrayCompactReturnType(SqlOperatorBinding opBinding)
   public static final SqlFunction ARRAY_CONTAINS =
       SqlBasicFunction.create(SqlKind.ARRAY_CONTAINS,
           ReturnTypes.BOOLEAN_NULLABLE,
-          OperandTypes.and(
-              OperandTypes.NONNULL_NONNULL_NOT_CAST,
-              OperandTypes.ARRAY_ELEMENT));
+          OperandTypes.ARRAY_ELEMENT_NONNULL);
 
   /** The "ARRAY_DISTINCT(array)" function. */
   @LibraryOperator(libraries = {SPARK})
@@ -1257,7 +1255,7 @@ private static RelDataType arrayCompactReturnType(SqlOperatorBinding opBinding)
       SqlBasicFunction.create(SqlKind.ARRAY_EXCEPT,
           ReturnTypes.LEAST_RESTRICTIVE,
           OperandTypes.and(
-              OperandTypes.NONNULL_NONNULL_NOT_CAST,
+              OperandTypes.NONNULL_NONNULL,
               OperandTypes.SAME_SAME,
               OperandTypes.family(SqlTypeFamily.ARRAY, SqlTypeFamily.ARRAY)));
 
@@ -1290,7 +1288,7 @@ private static RelDataType arrayInsertReturnType(SqlOperatorBinding opBinding) {
       SqlBasicFunction.create(SqlKind.ARRAY_INTERSECT,
           ReturnTypes.LEAST_RESTRICTIVE,
           OperandTypes.and(
-              OperandTypes.NONNULL_NONNULL_NOT_CAST,
+              OperandTypes.NONNULL_NONNULL,
               OperandTypes.SAME_SAME,
               OperandTypes.family(SqlTypeFamily.ARRAY, SqlTypeFamily.ARRAY)));
 

diff --git a/core/src/main/java/org/apache/calcite/sql/type/ArrayElementOperandTypeChecker.java b/core/src/main/java/org/apache/calcite/sql/type/ArrayElementOperandTypeChecker.java
@@ -21,6 +21,7 @@
 import org.apache.calcite.sql.SqlNode;
 import org.apache.calcite.sql.SqlOperandCountRange;
 import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlUtil;
 
 import com.google.common.collect.ImmutableList;
 
@@ -31,11 +32,41 @@
  * Parameter type-checking strategy where types must be Array and Array element type.
  */
 public class ArrayElementOperandTypeChecker implements SqlOperandTypeChecker {
+  //~ Instance fields --------------------------------------------------------
+
+  private final boolean allowNullCheck;
+  private final boolean allowCast;
+
+  //~ Constructors -----------------------------------------------------------
+
+  public ArrayElementOperandTypeChecker() {
+    this.allowNullCheck = false;
+    this.allowCast = false;
+  }
+
+  public ArrayElementOperandTypeChecker(boolean allowNullCheck, boolean allowCast) {
+    this.allowNullCheck = allowNullCheck;
+    this.allowCast = allowCast;
+  }
+
   //~ Methods ----------------------------------------------------------------
 
   @Override public boolean checkOperandTypes(
       SqlCallBinding callBinding,
       boolean throwOnFailure) {
+    if (allowNullCheck) {
+      // no operand can be null for type-checking to succeed
+      for (SqlNode node : callBinding.operands()) {
+        if (SqlUtil.isNullLiteral(node, allowCast)) {
+          if (throwOnFailure) {
+            throw callBinding.getValidator().newValidationError(node, RESOURCE.nullIllegal());
+          } else {
+            return false;
+          }
+        }
+      }
+    }
+
     final SqlNode op0 = callBinding.operand(0);
     if (!OperandTypes.ARRAY.checkSingleOperandType(
         callBinding,

diff --git a/core/src/main/java/org/apache/calcite/sql/type/NullOperandTypeChecker.java b/core/src/main/java/org/apache/calcite/sql/type/NullOperandTypeChecker.java
@@ -23,8 +23,7 @@
 import static org.apache.calcite.util.Static.RESOURCE;
 
 /**
- * Parameter type-checking strategy type must not be a NULL (including <code>NULL</code>,
- * <code>CAST(NULL as ...)</code> but not <code>CAST(CAST(NULL as ...) AS ...)</code>).
+ * Parameter type-checking strategy where all operand types must not be NULL.
  */
 public class NullOperandTypeChecker extends SameOperandTypeChecker {
   //~ Instance fields --------------------------------------------------------
@@ -42,7 +41,7 @@ public NullOperandTypeChecker(final int nOperands, final boolean allowCast) {
 
   @Override public boolean checkOperandTypes(final SqlCallBinding callBinding,
       final boolean throwOnFailure) {
-    // all operands can't be null
+    // no operand can be null for type-checking to succeed
     for (SqlNode node : callBinding.operands()) {
       if (SqlUtil.isNullLiteral(node, allowCast)) {
         if (throwOnFailure) {

diff --git a/core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java b/core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java
@@ -614,6 +614,9 @@ public static SqlOperandTypeChecker variadic(
   public static final SqlOperandTypeChecker ARRAY_ELEMENT =
       new ArrayElementOperandTypeChecker();
 
+  public static final SqlOperandTypeChecker ARRAY_ELEMENT_NONNULL =
+      new ArrayElementOperandTypeChecker(true, false);
+
   public static final SqlOperandTypeChecker ARRAY_INSERT =
       new ArrayInsertOperandTypeChecker();
 
@@ -639,9 +642,9 @@ public static SqlOperandTypeChecker variadic(
       new LiteralOperandTypeChecker(false);
 
   /**
-   * Operand type-checking strategy type must be a non-NULL value without cast.
+   * Operand type-checking strategy type must be a non-NULL value.
    */
-  public static final SqlSingleOperandTypeChecker NONNULL_NONNULL_NOT_CAST =
+  public static final SqlSingleOperandTypeChecker NONNULL_NONNULL =
       new NullOperandTypeChecker(2, false);
 
   /**

diff --git a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java b/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
@@ -6417,20 +6417,13 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
     // library (i.e. "fun=flink") we could add a function with Flink behavior.
     f.checkNull("array_contains(array[1, null], cast(null as integer))");
     f.checkType("array_contains(array[1, null], cast(null as integer))", "BOOLEAN");
-    f.checkFails("^array_contains(array[1, 2], true)^", "Cannot apply 'ARRAY_CONTAINS' to arguments"
-        + " of type 'ARRAY_CONTAINS\\(<INTEGER ARRAY>, <BOOLEAN>\\)'\\. Supported form\\(s\\): "
-        + "'ARRAY_CONTAINS\\(<EQUIVALENT_TYPE>, <EQUIVALENT_TYPE>\\)'", false);
+    f.checkFails("^array_contains(array[1, 2], true)^",
+        "INTEGER is not comparable to BOOLEAN", false);
 
     // check null without cast
-    f.checkFails("^array_contains(array[1, null], null)^", "Cannot apply 'ARRAY_CONTAINS' to "
-        + "arguments of type 'ARRAY_CONTAINS\\(<INTEGER ARRAY>, <NULL>\\)'\\. Supported form\\(s\\):"
-        + " 'ARRAY_CONTAINS\\(<EQUIVALENT_TYPE>, <EQUIVALENT_TYPE>\\)'", false);
-    f.checkFails("^array_contains(null, array[1, null])^", "Cannot apply 'ARRAY_CONTAINS' to "
-        + "arguments of type 'ARRAY_CONTAINS\\(<NULL>, <INTEGER ARRAY>\\)'\\. Supported form\\(s\\):"
-        + " 'ARRAY_CONTAINS\\(<EQUIVALENT_TYPE>, <EQUIVALENT_TYPE>\\)'", false);
-    f.checkFails("^array_contains(array[1, 2], null)^", "Cannot apply 'ARRAY_CONTAINS' to "
-        + "arguments of type 'ARRAY_CONTAINS\\(<INTEGER ARRAY>, <NULL>\\)'\\. Supported form\\(s\\):"
-        + " 'ARRAY_CONTAINS\\(<EQUIVALENT_TYPE>, <EQUIVALENT_TYPE>\\)'", false);
+    f.checkFails("array_contains(array[1, 2], ^null^)", "Illegal use of 'NULL'", false);
+    f.checkFails("array_contains(^null^, array[1, 2])", "Illegal use of 'NULL'", false);
+    f.checkFails("array_contains(^null^, null)", "Illegal use of 'NULL'", false);
   }
 
   /** Tests {@code ARRAY_DISTINCT} function from Spark. */
@@ -6801,6 +6794,10 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
         "Cannot apply 'ARRAY_EXCEPT' to arguments of type 'ARRAY_EXCEPT\\(<NULL>, "
             + "<INTEGER ARRAY>\\)'\\. Supported form\\(s\\): 'ARRAY_EXCEPT\\(<EQUIVALENT_TYPE>, "
             + "<EQUIVALENT_TYPE>\\)'", false);
+    f.checkFails("^array_except(null, null)^",
+        "Cannot apply 'ARRAY_EXCEPT' to arguments of type 'ARRAY_EXCEPT\\(<NULL>, "
+            + "<NULL>\\)'\\. Supported form\\(s\\): 'ARRAY_EXCEPT\\(<EQUIVALENT_TYPE>, "
+            + "<EQUIVALENT_TYPE>\\)'", false);
   }
 
   /** Tests {@code ARRAY_INSERT} function from Spark. */
@@ -6906,6 +6903,10 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
         "Cannot apply 'ARRAY_INTERSECT' to arguments of type 'ARRAY_INTERSECT\\(<NULL>, "
             + "<INTEGER ARRAY>\\)'\\. Supported form\\(s\\): 'ARRAY_INTERSECT\\(<EQUIVALENT_TYPE>, "
             + "<EQUIVALENT_TYPE>\\)'", false);
+    f.checkFails("^array_intersect(null, null)^",
+        "Cannot apply 'ARRAY_INTERSECT' to arguments of type 'ARRAY_INTERSECT\\(<NULL>, "
+            + "<NULL>\\)'\\. Supported form\\(s\\): 'ARRAY_INTERSECT\\(<EQUIVALENT_TYPE>, "
+            + "<EQUIVALENT_TYPE>\\)'", false);
   }
 
   /** Tests {@code ARRAY_UNION} function from Spark. */