[CALCITE-6764] Field access from a nullable ROW should be nullable (p…

…art 2)

Signed-off-by: Mihai Budiu <[email protected]>

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java

@@ -179,18 +179,37 @@ RelDataType createMultisetType(
    * Creates a type that is the same as another type but with possibly
    * different nullability. The output type may be identical to the input
    * type. For type systems without a concept of nullability, the return value
-   * is always the same as the input.
+   * is always the same as the input. This function never returns a nullable
+   * record type; when applied to a record type, it recursively makes all fields
+   * nullable and the record itself is non-nullable.
    *
-   * @param type     input type
-   * @param nullable true to request a nullable type; false to request a NOT
-   *                 NULL type
-   * @return output type, same as input type except with specified nullability
+   * @param type input type
+   * @param nullable true to request a nullable type; false to request a NOT NULL type.
+   * @return output type, same as input type except with specified nullability (see description
+   * for record types).
    * @throws NullPointerException if type is null
    */
   RelDataType createTypeWithNullability(
       RelDataType type,
       boolean nullable);
 
+  /**
+   * Creates a type that is the same as another type but with possibly
+   * different nullability. The output type may be identical to the input
+   * type. For type systems without a concept of nullability, the return value
+   * is always the same as the input. This differs from {@link
+   * #createTypeWithNullability(RelDataType, boolean)} in the handling of record
+   * types.  This function returns a nullable struct.
+   *
+   * @param type input type
+   * @param nullable true to request a nullable type; false to request a NOT NULL type.
+   * @return output type, same as input type except with specified nullability
+   * @throws NullPointerException if type is null
+   */
+  RelDataType enforceTypeWithNullability(
+      RelDataType type,
+      boolean nullable);
+
   /**
    * Creates a type that is the same as another type but with possibly
    * different charset or collation. For types without a concept of charset or

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java

@@ -410,6 +410,31 @@ private RelDataType copyRecordType(
     return canonize(newType);
   }
 
+  @Override public RelDataType enforceTypeWithNullability(
+      final RelDataType type,
+      final boolean nullable) {
+    requireNonNull(type, "type");
+    RelDataType newType;
+    if (type.isNullable() == nullable) {
+      newType = type;
+    } else if (type instanceof RelRecordType) {
+      return createStructType(type.getStructKind(),
+          new AbstractList<RelDataType>() {
+            @Override public RelDataType get(int index) {
+              return type.getFieldList().get(index).getType();
+            }
+
+            @Override public int size() {
+              return type.getFieldCount();
+            }
+          },
+          type.getFieldNames(), nullable);
+    } else {
+      newType = copySimpleType(type, nullable);
+    }
+    return canonize(newType);
+  }
+
   /**
    * Registers a type, or returns the existing type if it is already
    * registered.

core/src/main/java/org/apache/calcite/rex/RexBuilder.java

@@ -257,7 +257,7 @@ private RexNode makeFieldAccessInternal(
     }
 
     if (expr.getType().isNullable()) {
-      fieldType = typeFactory.createTypeWithNullability(fieldType, true);
+      fieldType = typeFactory.enforceTypeWithNullability(fieldType, true);
     }
     return new RexFieldAccess(expr, field, fieldType);
   }

core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java

@@ -126,7 +126,7 @@ protected static void checkPrecScale(SqlTypeName typeName,
   /**
    * Constructs a type with nullablity.
    */
-  BasicSqlType createWithNullability(boolean nullable) {
+  public BasicSqlType createWithNullability(boolean nullable) {
     if (nullable == this.isNullable) {
       return this;
     }

core/src/main/java/org/apache/calcite/sql/type/SqlTypeFactoryImpl.java

@@ -253,6 +253,28 @@ public SqlTypeFactoryImpl(RelDataTypeSystem typeSystem) {
     return canonize(newType);
   }
 
+  @Override public RelDataType enforceTypeWithNullability(
+      final RelDataType type,
+      final boolean nullable) {
+    final RelDataType newType;
+    if (type instanceof BasicSqlType) {
+      newType = ((BasicSqlType) type).createWithNullability(nullable);
+    } else if (type instanceof MapSqlType) {
+      newType = copyMapType(type, nullable);
+    } else if (type instanceof ArraySqlType) {
+      newType = copyArrayType(type, nullable);
+    } else if (type instanceof MultisetSqlType) {
+      newType = copyMultisetType(type, nullable);
+    } else if (type instanceof IntervalSqlType) {
+      newType = copyIntervalType(type, nullable);
+    } else if (type instanceof ObjectSqlType) {
+      newType = copyObjectType(type, nullable);
+    } else {
+      return super.enforceTypeWithNullability(type, nullable);
+    }
+    return canonize(newType);
+  }
+
   private static void assertBasic(SqlTypeName typeName) {
     assert typeName != null;
     assert typeName != SqlTypeName.MULTISET

core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java

@@ -7166,7 +7166,15 @@ private class DeriveTypeVisitor implements SqlVisitor<RelDataType> {
           throw newValidationError(id.getComponent(i),
               RESOURCE.unknownField(name));
         }
+        boolean recordIsNullable = type.isNullable();
         type = field.getType();
+        if (recordIsNullable) {
+          // If parent record is nullable, field must also be nullable.
+          // Consider CREATE TABLE T(p ROW(k VARCHAR) NULL);
+          // Since T.p is nullable, T.p.k also has to be nullable, even if
+          // the type of k itself is not nullable.
+          type = getTypeFactory().enforceTypeWithNullability(type, true);
+        }
       }
       type =
           SqlTypeUtil.addCharsetAndCollation(

core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java

@@ -87,29 +87,13 @@ class TableInRootSchemaTest {
     connection.close();
   }
 
-  /**
-   * Helper class for the test for [CALCITE-6764] below.
-   */
-  private static class RowTable extends AbstractQueryableTable {
-    protected RowTable() {
+  /** Represents a table with no data. An abstract base class,
+   * derived classes need to define the schema. */
+  private abstract static class EmptyTable extends AbstractQueryableTable {
+    protected EmptyTable() {
       super(Object[].class);
     }
 
-    @Override public RelDataType getRowType(RelDataTypeFactory typeFactory) {
-      final PairList<String, RelDataType> columnDesc = PairList.withCapacity(1);
-      // Schema contains a column whose type is MAP<VARCHAR, ROW(VARCHAR)>, but
-      // the ROW type can be nullable.
-      final RelDataType colType =
-          typeFactory.createMapType(typeFactory.createSqlType(SqlTypeName.VARCHAR),
-            new RelRecordType(
-                StructKind.PEEK_FIELDS,
-                  ImmutableList.of(
-                      new RelDataTypeFieldImpl("K", 0,
-                          typeFactory.createSqlType(SqlTypeName.VARCHAR))), true));
-      columnDesc.add("P", colType);
-      return typeFactory.createStructType(columnDesc);
-    }
-
     @Override public <T> Queryable<T> asQueryable(
         QueryProvider queryProvider, SchemaPlus schema, String tableName) {
       return new AbstractTableQueryable<T>(queryProvider, schema, this,
@@ -134,14 +118,101 @@ protected RowTable() {
     }
   }
 
-  /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6764">[CALCITE-6764]
+  /** Helper class for the test for [CALCITE-6764] below. */
+  private static class TableWithNullableRowInMap extends EmptyTable {
+    protected TableWithNullableRowInMap() {
+      super();
+    }
+
+    @Override public RelDataType getRowType(RelDataTypeFactory typeFactory) {
+      final PairList<String, RelDataType> columnDesc = PairList.withCapacity(1);
+      // Schema contains a column whose type is MAP<VARCHAR, ROW(VARCHAR)>, but
+      // the ROW type can be nullable.  This can conceivably be created by a
+      // declaration such as
+      // CREATE TABLE T(P MAP<VARCHAR, ROW(K VARCHAR NON NULL, S VARCHAR NULL)>);
+      final RelDataType colType =
+          typeFactory.createMapType(typeFactory.createSqlType(SqlTypeName.VARCHAR),
+            new RelRecordType(
+                StructKind.PEEK_FIELDS,
+                  ImmutableList.of(
+                      new RelDataTypeFieldImpl("K", 0,
+                          typeFactory.createSqlType(SqlTypeName.VARCHAR)),
+                      new RelDataTypeFieldImpl("S", 1,
+                          typeFactory.createTypeWithNullability(
+                              typeFactory.createSqlType(SqlTypeName.VARCHAR), true))),
+                true));
+      columnDesc.add("P", colType);
+      return typeFactory.createStructType(columnDesc);
+    }
+  }
+
+  /** Helper class for the test for [CALCITE-6764] below. */
+  private static class TableWithNullableRowToplevel extends EmptyTable {
+    protected TableWithNullableRowToplevel() {
+      super();
+    }
+
+    @Override public RelDataType getRowType(RelDataTypeFactory typeFactory) {
+      final PairList<String, RelDataType> columnDesc = PairList.withCapacity(1);
+      // This table can conceivably be created by a declaration such as
+      // CREATE TABLE T(P ROW(K VARCHAR NOT NULL) NULL,
+      //                Q ROW(S ROW(L VARCHAR NOT NULL, M VARCHAR NULL) NULL) NULL);
+      final RelDataType pColType =
+          new RelRecordType(
+              StructKind.PEEK_FIELDS, ImmutableList.of(
+                  new RelDataTypeFieldImpl(
+                      "K", 0, typeFactory.createSqlType(SqlTypeName.VARCHAR))),
+          true);
+      final RelDataType sType =
+          new RelRecordType(
+              StructKind.PEEK_FIELDS, ImmutableList.of(
+              new RelDataTypeFieldImpl(
+                  "L", 0, typeFactory.createSqlType(SqlTypeName.VARCHAR)),
+              new RelDataTypeFieldImpl(
+                  "M", 1, typeFactory.createSqlType(SqlTypeName.VARCHAR))),
+              false);
+      final RelDataType qColType =
+          new RelRecordType(
+              StructKind.PEEK_FIELDS, ImmutableList.of(
+              new RelDataTypeFieldImpl("S", 0, sType)),
+              true);
+      columnDesc.add("P", pColType);
+      columnDesc.add("Q", qColType);
+      return typeFactory.createStructType(columnDesc);
+    }
+  }
+
+  /** Test case for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-6764">[CALCITE-6764]
+   * Field access from a nullable ROW should be nullable</a>. */
+  @Test void testNullableRowInMap() throws Exception {
+    Connection connection = DriverManager.getConnection("jdbc:calcite:");
+    CalciteConnection calciteConnection = connection.unwrap(CalciteConnection.class);
+    calciteConnection.getRootSchema().add("T", new TableWithNullableRowInMap());
+    Statement statement = calciteConnection.createStatement();
+    // Without the fix to this issue the Validator crashes with an AssertionFailure:
+    // java.lang.RuntimeException: java.lang.AssertionError:
+    // Conversion to relational algebra failed to preserve datatypes:
+    // validated type:
+    ResultSet resultSet = statement.executeQuery("SELECT P['a'].K, P['a'].S FROM T");
+    resultSet.close();
+    statement.close();
+    connection.close();
+  }
+
+  /** Test case for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-6764">[CALCITE-6764]
    * Field access from a nullable ROW should be nullable</a>. */
-  @Test void testNullableValue() throws Exception {
+  @Test void testNullableRowTopLevel() throws Exception {
     Connection connection = DriverManager.getConnection("jdbc:calcite:");
     CalciteConnection calciteConnection = connection.unwrap(CalciteConnection.class);
-    calciteConnection.getRootSchema().add("T", new RowTable());
+    calciteConnection.getRootSchema().add("T", new TableWithNullableRowToplevel());
     Statement statement = calciteConnection.createStatement();
-    ResultSet resultSet = statement.executeQuery("SELECT P['a'].K FROM T");
+    // Without the fix to this issue the Validator crashes with an AssertionFailure:
+    // java.lang.RuntimeException: java.lang.AssertionError:
+    // Conversion to relational algebra failed to preserve datatypes:
+    // validated type:
+    ResultSet resultSet = statement.executeQuery("SELECT T.P.K, T.Q.S, T.Q.S.L, T.Q.S.M FROM T");
     resultSet.close();
     statement.close();
     connection.close();

site/_docs/history.md

@@ -49,6 +49,16 @@ other software versions as specified in gradle.properties.
 #### Breaking Changes
 {: #breaking-1-39-0}
 
+*RelDataTypeFactory interface*.  The fix for [<a
+href="https://issues.apache.org/jira/browse/CALCITE-6764">CALCITE-6764</a>]
+introduces a new method
+`RelDataTypeFactory#enforceTypeWithNullability` in the existing
+`RelDataTypeFactory` interface.  The behavior of the new function is
+similar to the existing API `createTypeWithNullability`; however, the
+existing implementations of the `createTypeWithNullability` API cannot
+create nullable record (`ROW`) types.  Nullable record types are
+legitimate in several SQL dialects.
+
 *Checked arithmetic*.  [<a
 href="https://issues.apache.org/jira/browse/CALCITE-6685">CALCITE-6685</a>]
 introduces support for checked arithmetic on short integer types.  A