Address review comments

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

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

@@ -7085,6 +7085,9 @@ private class DeriveTypeVisitor implements SqlVisitor<RelDataType> {
         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().createTypeWithNullability(type, true);
         }
       }

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

@@ -87,7 +87,7 @@ class TableInRootSchemaTest {
     connection.close();
   }
 
-  /** Represents a table with no data.  An abstract base class,
+  /** 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() {
@@ -119,8 +119,8 @@ protected EmptyTable() {
   }
 
   /** Helper class for the test for [CALCITE-6764] below. */
-  private static class RowTable extends EmptyTable {
-    protected RowTable() {
+  private static class TableWithNullableRowInMap extends EmptyTable {
+    protected TableWithNullableRowInMap() {
       super();
     }
 
@@ -143,8 +143,8 @@ protected RowTable() {
   }
 
   /** Helper class for the test for [CALCITE-6764] below. */
-  private static class RowTable2 extends EmptyTable {
-    protected RowTable2() {
+  private static class TableWithNullableRowToplevel extends EmptyTable {
+    protected TableWithNullableRowToplevel() {
       super();
     }
 
@@ -163,26 +163,36 @@ protected RowTable2() {
     }
   }
 
-  /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6764">[CALCITE-6764]
+  /** 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 {
     Connection connection = DriverManager.getConnection("jdbc:calcite:");
     CalciteConnection calciteConnection = connection.unwrap(CalciteConnection.class);
-    calciteConnection.getRootSchema().add("T", new RowTable());
+    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 FROM T");
     resultSet.close();
     statement.close();
     connection.close();
   }
 
-  /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6764">[CALCITE-6764]
+  /** 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 testNullableValue2() throws Exception {
     Connection connection = DriverManager.getConnection("jdbc:calcite:");
     CalciteConnection calciteConnection = connection.unwrap(CalciteConnection.class);
-    calciteConnection.getRootSchema().add("T", new RowTable2());
+    calciteConnection.getRootSchema().add("T", new TableWithNullableRowToplevel());
     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 t.p.k FROM T");
     resultSet.close();
     statement.close();