[CALCITE-4510] RexLiteral can produce wrong digest for some user defi…

…ned types
apache · Apr 30, 2021 · 350802b · 350802b
1 parent d7b3c83
commit 350802b
Show file tree

Hide file tree

Showing 10 changed files with 54 additions and 15 deletions.
diff --git a/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java b/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
@@ -135,6 +135,8 @@
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
+import static org.apache.calcite.rel.type.RelDataTypeImpl.NON_NULLABLE_SUFFIX;
+
 /**
  * <code>RelOptUtil</code> defines static utility methods for use in optimizing
  * {@link RelNode}s.
@@ -4315,7 +4317,7 @@ void accept(RelDataType type) {
         this.indent = prevIndent;
         pw.print(")");
         if (!type.isNullable()) {
-          pw.print(" NOT NULL");
+          pw.print(NON_NULLABLE_SUFFIX);
         }
       } else if (type instanceof MultisetSqlType) {
         // E.g. "INTEGER NOT NULL MULTISET NOT NULL"
@@ -4326,7 +4328,7 @@ void accept(RelDataType type) {
         accept(componentType);
         pw.print(" MULTISET");
         if (!type.isNullable()) {
-          pw.print(" NOT NULL");
+          pw.print(NON_NULLABLE_SUFFIX);
         }
       } else {
         // E.g. "INTEGER" E.g. "VARCHAR(240) CHARACTER SET "ISO-8859-1"

diff --git a/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java b/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
@@ -50,6 +50,12 @@
  */
 public abstract class RelDataTypeImpl
     implements RelDataType, RelDataTypeFamily {
+
+  /**
+   * Suffix for the digests of non-nullable types.
+   */
+  public static final String NON_NULLABLE_SUFFIX = " NOT NULL";
+
   //~ Instance fields --------------------------------------------------------
 
   protected final @Nullable List<RelDataTypeField> fieldList;
@@ -279,7 +285,7 @@ protected void computeDigest(
     StringBuilder sb = new StringBuilder();
     generateTypeString(sb, true);
     if (!isNullable()) {
-      sb.append(" NOT NULL");
+      sb.append(NON_NULLABLE_SUFFIX);
     }
     digest = sb.toString();
   }

diff --git a/core/src/main/java/org/apache/calcite/rex/RexLiteral.java b/core/src/main/java/org/apache/calcite/rex/RexLiteral.java
@@ -65,6 +65,7 @@
 import java.util.TimeZone;
 
 import static org.apache.calcite.linq4j.Nullness.castNonNull;
+import static org.apache.calcite.rel.type.RelDataTypeImpl.NON_NULLABLE_SUFFIX;
 
 import static java.util.Objects.requireNonNull;
 
@@ -432,11 +433,13 @@ private static String toJavaString(
     if (includeType != RexDigestIncludeType.NO_TYPE) {
       sb.append(':');
       final String fullTypeString = type.getFullTypeString();
-      if (!fullTypeString.endsWith("NOT NULL")) {
+
+      if (!fullTypeString.endsWith(NON_NULLABLE_SUFFIX)) {
         sb.append(fullTypeString);
       } else {
         // Trim " NOT NULL". Apparently, the literal is not null, so we just print the data type.
-        sb.append(fullTypeString, 0, fullTypeString.length() - 9);
+        sb.append(fullTypeString, 0,
+            fullTypeString.length() - NON_NULLABLE_SUFFIX.length());
       }
     }
     return sb.toString();

diff --git a/core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java b/core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
@@ -60,6 +60,7 @@
 import java.util.function.Function;
 import java.util.stream.Collectors;
 
+import static org.apache.calcite.rel.type.RelDataTypeImpl.NON_NULLABLE_SUFFIX;
 import static org.apache.calcite.sql.type.NonNullableAccessors.getCharset;
 import static org.apache.calcite.sql.type.NonNullableAccessors.getCollation;
 import static org.apache.calcite.sql.type.NonNullableAccessors.getComponentTypeOrThrow;
@@ -1194,8 +1195,8 @@ public static boolean equalSansNullability(RelDataType type1, RelDataType type2)
     }
 
     return (x.length() == y.length()
-        || x.length() == y.length() + 9 && x.endsWith(" NOT NULL"))
-        && x.startsWith(y);
+        || x.length() == y.length() + NON_NULLABLE_SUFFIX.length()
+        && x.endsWith(NON_NULLABLE_SUFFIX)) && x.startsWith(y);
   }
 
   /**

diff --git a/core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java b/core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java
@@ -22,6 +22,7 @@
 import org.apache.calcite.rel.type.RelDataTypeFactory;
 import org.apache.calcite.rel.type.RelDataTypeField;
 import org.apache.calcite.rel.type.RelDataTypeFieldImpl;
+import org.apache.calcite.rel.type.RelDataTypeImpl;
 import org.apache.calcite.rel.type.RelDataTypeSystem;
 import org.apache.calcite.rel.type.RelDataTypeSystemImpl;
 import org.apache.calcite.sql.SqlCollation;
@@ -792,4 +793,23 @@ private void checkBigDecimalLiteral(RexBuilder builder, String val) {
     RexChecker checker = new RexChecker(structType, () -> null, Litmus.THROW);
     assertThat(fieldAccess.accept(checker), is(true));
   }
+
+  /** Emulate a user defined type. */
+  private static class UDT extends RelDataTypeImpl {
+    UDT() {
+      this.digest = "(udt)NOT NULL";
+    }
+
+    @Override protected void generateTypeString(StringBuilder sb, boolean withDetail) {
+      sb.append("udt");
+    }
+  }
+
+  @Test void testUDTLiteralDigest() {
+    RexLiteral literal = new RexLiteral(new BigDecimal(0L), new UDT(), SqlTypeName.BIGINT);
+
+    // when the space before "NOT NULL" is missing, the digest is not correct
+    // and the suffix should not be removed.
+    assertThat(literal.digest, is("0L:(udt)NOT NULL"));
+  }
 }
diff --git a/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java b/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
@@ -23,6 +23,7 @@
 import org.apache.calcite.rel.metadata.NullSentinel;
 import org.apache.calcite.rel.type.RelDataType;
 import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeImpl;
 import org.apache.calcite.sql.SqlKind;
 import org.apache.calcite.sql.SqlOperator;
 import org.apache.calcite.sql.SqlSpecialOperator;
@@ -2524,7 +2525,8 @@ private void assertTypeAndToString(
       RexNode rexNode, String representation, String type) {
     assertEquals(representation, rexNode.toString());
     assertEquals(type, rexNode.getType().toString()
-        + (rexNode.getType().isNullable() ? "" : " NOT NULL"), "type of " + rexNode);
+        + (rexNode.getType().isNullable() ? "" : RelDataTypeImpl.NON_NULLABLE_SUFFIX),
+        "type of " + rexNode);
   }
 
   @Test void testIsDeterministic() {

diff --git a/core/src/test/java/org/apache/calcite/rex/RexProgramTestBase.java b/core/src/test/java/org/apache/calcite/rex/RexProgramTestBase.java
@@ -17,6 +17,7 @@
 package org.apache.calcite.rex;
 
 import org.apache.calcite.plan.RelOptPredicateList;
+import org.apache.calcite.rel.type.RelDataTypeImpl;
 import org.apache.calcite.sql.SqlKind;
 import org.apache.calcite.sql.type.SqlTypeName;
 import org.apache.calcite.test.Matchers;
@@ -78,7 +79,8 @@ protected void assertNode(String message, String expected, RexNode node) {
       // toString contains type (see RexCall.toString)
       actual = node.toString();
     } else {
-      actual = node + ":" + node.getType() + (node.getType().isNullable() ? "" : " NOT NULL");
+      actual = node + ":" + node.getType() + (node.getType().isNullable() ? ""
+          : RelDataTypeImpl.NON_NULLABLE_SUFFIX);
     }
     assertEquals(expected, actual, message);
   }

diff --git a/core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java b/core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
@@ -93,6 +93,7 @@
 import java.util.regex.Pattern;
 import java.util.stream.Stream;
 
+import static org.apache.calcite.rel.type.RelDataTypeImpl.NON_NULLABLE_SUFFIX;
 import static org.apache.calcite.sql.fun.SqlStdOperatorTable.PI;
 import static org.apache.calcite.util.DateTimeStringUtils.getDateFormatter;
 
@@ -468,7 +469,7 @@ private void checkCastToApproxOkay(
       double delta) {
     tester.checkScalarApprox(
         getCastString(value, targetType, false),
-        targetType + " NOT NULL",
+        targetType + NON_NULLABLE_SUFFIX,
         expected,
         delta);
   }
@@ -480,7 +481,7 @@ private void checkCastToStringOkay(
     tester.checkString(
         getCastString(value, targetType, false),
         expected,
-        targetType + " NOT NULL");
+        targetType + NON_NULLABLE_SUFFIX);
   }
 
   private void checkCastToScalarOkay(
@@ -489,7 +490,7 @@ private void checkCastToScalarOkay(
       String expected) {
     tester.checkScalarExact(
         getCastString(value, targetType, false),
-        targetType + " NOT NULL",
+        targetType + NON_NULLABLE_SUFFIX,
         expected);
   }
 
@@ -7040,7 +7041,7 @@ void assertSubFunReturns(boolean binary, String s, int start,
         if (binary) {
           expected = DOUBLER.apply(expected);
         }
-        t.checkString(expression, expected, type + " NOT NULL");
+        t.checkString(expression, expected, type + NON_NULLABLE_SUFFIX);
       }
     }
   }

diff --git a/core/src/test/java/org/apache/calcite/sql/test/SqlTests.java b/core/src/test/java/org/apache/calcite/sql/test/SqlTests.java
@@ -18,6 +18,7 @@
 
 import org.apache.calcite.avatica.ColumnMetaData;
 import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeImpl;
 import org.apache.calcite.runtime.CalciteContextException;
 import org.apache.calcite.sql.parser.SqlParseException;
 import org.apache.calcite.sql.parser.SqlParserUtil;
@@ -104,7 +105,7 @@ public static String getTypeString(RelDataType sqlType) {
         actual = actual + "(" + sqlType.getPrecision() + ")";
       }
       if (!sqlType.isNullable()) {
-        actual += " NOT NULL";
+        actual += RelDataTypeImpl.NON_NULLABLE_SUFFIX;
       }
       return actual;
 

diff --git a/core/src/test/java/org/apache/calcite/test/CalciteAssert.java b/core/src/test/java/org/apache/calcite/test/CalciteAssert.java
@@ -38,6 +38,7 @@
 import org.apache.calcite.rel.RelRoot;
 import org.apache.calcite.rel.type.RelDataType;
 import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeImpl;
 import org.apache.calcite.runtime.CalciteException;
 import org.apache.calcite.runtime.FlatLists;
 import org.apache.calcite.runtime.GeoFunctions;
@@ -487,7 +488,7 @@ private static String typeString(ResultSetMetaData metaData)
               + " "
               + metaData.getColumnTypeName(i + 1)
               + (metaData.isNullable(i + 1) == ResultSetMetaData.columnNoNulls
-              ? " NOT NULL"
+              ? RelDataTypeImpl.NON_NULLABLE_SUFFIX
               : ""));
     }
     return list.toString();