asolimando commented on code in PR #4139:
URL: https://github.com/apache/calcite/pull/4139#discussion_r1913735661
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7081,7 +7081,12 @@ 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.
Review Comment:
`The type of p['a'] is nullable, so p['a'].k should also be nullable.`
I think this would make a great comment (it can replace or be added to the
existing one)
##########
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,
Review Comment:
```suggestion
/** Represents a table with no data. An abstract base class,
```
(nit: extra space)
##########
core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java:
##########
@@ -146,4 +175,17 @@ protected RowTable() {
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>. */
Review Comment:
I don't see where we are actually testing the result being `NULL`?
##########
core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java:
##########
@@ -134,6 +118,51 @@ protected RowTable() {
}
}
+ /** Helper class for the test for [CALCITE-6764] below. */
+ private static class RowTable extends EmptyTable {
+ protected RowTable() {
+ 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)>);
+ 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);
+ }
+ }
+
+ /** Helper class for the test for [CALCITE-6764] below. */
+ private static class RowTable2 extends EmptyTable {
Review Comment:
Nit: the difference between `RowTable` and `RowTable2` seems to be in the
nullability of the map composing the row type, can we make this explicit by
calling them `NullableRowTable` and `RowTable` or something along this line?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
