From 3e6b7b1d00f9b3fe41c02638d4fbbfd7971f99eb Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Wed, 23 Sep 2026 10:06:08 +0300 Subject: [PATCH 1/3] fix(isthmus)!: keep a struct's declared field nullability in Calcite A Substrait struct declares its own nullability and its fields' separately. TypeConverter applied the struct's with createTypeWithNullability, which Calcite documents as widening a record's fields along with the record, so struct? came back as struct?. Apply it with enforceTypeWithNullability, which the factory canonizes the same way but which leaves each field as declared. A computed field inside a nullable struct now converts back instead of reporting that the row type cannot say what the schema said. Closes #1154 --- .../io/substrait/isthmus/TypeConverter.java | 5 +++- .../io/substrait/isthmus/CalciteTypeTest.java | 19 +++++++++++++++ .../isthmus/SchemaCollectorTest.java | 4 ++-- .../isthmus/VirtualTableScanTest.java | 24 +++++++------------ 4 files changed, 34 insertions(+), 18 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java index 88f0b0a6a..ee00247d5 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java @@ -535,7 +535,10 @@ private RelDataType t(boolean nullable, SqlTypeName typeName, Integer... props) } private RelDataType n(Type substraitType, RelDataType type) { - return typeFactory.createTypeWithNullability(type, n(substraitType)); + // enforceTypeWithNullability, not createTypeWithNullability: the latter + // widens a record's fields along with the record, which loses the + // nullability the Substrait type declares for them. + return typeFactory.enforceTypeWithNullability(type, n(substraitType)); } } } diff --git a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java index 138b99cd3..24b5ee2e1 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java @@ -1,6 +1,7 @@ package io.substrait.isthmus; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -320,6 +321,24 @@ void nestedStruct() { "topStruct1", "inner1", "inner2", "topStruct2", "inner3", "inner4", "topVarChar")); } + /** + * A struct declares its own nullability and its fields' separately, and a trip through Calcite + * keeps both: {@code createTypeWithNullability} widens a record's fields along with the record, + * which dropped what the schema said about them. + */ + @Test + void aNullableStructKeepsWhatItsFieldsDeclare() { + Type.Struct substrait = + TypeCreator.NULLABLE.struct(TypeCreator.REQUIRED.I32, TypeCreator.NULLABLE.FP64); + + RelDataType calcite = TypeConverter.DEFAULT.toCalcite(type, substrait, List.of("a", "b")); + + assertTrue(calcite.isNullable()); + assertFalse(calcite.getFieldList().get(0).getType().isNullable()); + assertTrue(calcite.getFieldList().get(1).getType().isNullable()); + assertEquals(substrait, TypeConverter.DEFAULT.toSubstrait(calcite)); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) void userDefinedType(boolean nullable) { diff --git a/isthmus/src/test/java/io/substrait/isthmus/SchemaCollectorTest.java b/isthmus/src/test/java/io/substrait/isthmus/SchemaCollectorTest.java index c14d6737e..18f379826 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SchemaCollectorTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SchemaCollectorTest.java @@ -122,9 +122,9 @@ void testUpdate() { CalciteSchema calciteSchema = schemaCollector.toSchema(rel); CalciteSchema schema1 = calciteSchema.getSubSchema("schema1", false); - hasTable(schema1, "table1", "RecordType(BOOLEAN col1)"); + hasTable(schema1, "table1", "RecordType(BOOLEAN NOT NULL col1)"); - hasTable(schema1, "table2", "RecordType(BOOLEAN col1)"); + hasTable(schema1, "table2", "RecordType(BOOLEAN NOT NULL col1)"); } @Test diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java index 5373da93e..989793e99 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java @@ -190,9 +190,8 @@ void twoStructRowsConvert() { /** * A schema struct that is itself nullable says nothing about the relation: a row type describes - * the columns, and Calcite derives one NOT NULL everywhere else. What the nullability does reach - * is the columns -- Calcite makes a struct's fields nullable along with the struct -- which is - * the same row type the conversion built before it was given the schema's names. + * the columns, and Calcite derives one NOT NULL everywhere else. Its fields keep what the schema + * declares for them, so a required column stays NOT NULL inside a nullable struct. */ @Test void nullableSchemaStructGivesANotNullRowType() { @@ -200,7 +199,8 @@ void nullableSchemaStructGivesANotNullRowType() { VirtualTableScan virtualTableScan = virtualTable(schema, List.of(sb.i32(1))); RelNode relNode = substraitToCalcite.convert(virtualTableScan); - assertEquals("RecordType(INTEGER col1) NOT NULL", relNode.getRowType().getFullTypeString()); + assertEquals( + "RecordType(INTEGER NOT NULL col1) NOT NULL", relNode.getRowType().getFullTypeString()); } /** @@ -401,13 +401,11 @@ void structInMapColumnConverts() { } /** - * A computed field inside a nullable struct is where the row type stops being able to say what - * the schema said: Calcite pushes the struct's nullability into its fields, and a value built - * from expressions takes its type from them, so the trip back cannot rebuild the declared type. - * Reported here rather than as a type mismatch from {@link VirtualTableScan}'s own check. + * A computed field inside a nullable struct: the row type keeps what the schema declared for the + * fields, so the trip back rebuilds the declared type rather than reporting that it cannot. */ @Test - void aComputedFieldInsideANullableStructIsReportedOnTheWayBack() { + void aComputedFieldInsideANullableStructConvertsBack() { NamedStruct schema = NamedStruct.of(List.of("outer", "a", "b"), R.struct(N.struct(R.I32, R.FP64))); VirtualTableScan virtualTableScan = @@ -418,12 +416,8 @@ void aComputedFieldInsideANullableStructIsReportedOnTheWayBack() { true, List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(2.0))))); RelNode relNode = substraitToCalcite.convert(virtualTableScan); - assertTrue( - assertThrows( - UnsupportedOperationException.class, - () -> SubstraitRelVisitor.convert(relNode, converterProvider)) - .getMessage() - .contains("does not carry its column's type")); + assertEquals( + schema.struct(), SubstraitRelVisitor.convert(relNode, converterProvider).getRecordType()); } /** From 6b4df48610933abbc2286c7c4c7dfd0eba173433 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Mon, 28 Sep 2026 23:42:22 +0300 Subject: [PATCH 2/3] fix(isthmus): give a collected table a NOT NULL row type A nullable schema struct became a nullable Calcite record with NOT NULL columns. createTypeWithNullability returns such a type unchanged when asked for nullable, so outer-join widening kept the null-generating side's columns NOT NULL. Normalize the outermost struct as the virtual-table path already does; type.proto asks for a required outermost struct anyway. --- .../io/substrait/isthmus/SchemaCollector.java | 7 ++++- .../isthmus/SchemaCollectorTest.java | 28 +++++++++++++++++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java b/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java index c4874ba79..0cf23b739 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java @@ -71,8 +71,13 @@ public CalciteSchema toSchema(@NonNull final Rel rel) { // Create the table if it is not present CalciteSchema.TableEntry table = schema.getTable(tableName, CASE_SENSITIVE); if (table == null) { + // A relation's row type says what its columns are, not whether a value is there, and + // Calcite builds one NOT NULL wherever it derives one -- so a schema struct contributes + // its fields and not its own nullability. RelDataType rowType = - typeConverter.toCalcite(typeFactory, namedStruct.struct(), namedStruct.names()); + typeFactory.createTypeWithNullability( + typeConverter.toCalcite(typeFactory, namedStruct.struct(), namedStruct.names()), + false); schema.add(tableName, new SubstraitTable(tableName, rowType)); } } diff --git a/isthmus/src/test/java/io/substrait/isthmus/SchemaCollectorTest.java b/isthmus/src/test/java/io/substrait/isthmus/SchemaCollectorTest.java index 18f379826..1b6e5c959 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SchemaCollectorTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SchemaCollectorTest.java @@ -10,12 +10,16 @@ import io.substrait.expression.ImmutableFieldReference; import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.relation.AbstractWriteRel; +import io.substrait.relation.Join; +import io.substrait.relation.NamedScan; import io.substrait.relation.NamedUpdate; import io.substrait.relation.Rel; +import io.substrait.type.NamedStruct; import io.substrait.type.TypeCreator; import java.util.Arrays; import java.util.List; import org.apache.calcite.jdbc.CalciteSchema; +import org.apache.calcite.rel.RelNode; import org.junit.jupiter.api.Test; class SchemaCollectorTest extends PlanTestBase { @@ -122,9 +126,9 @@ void testUpdate() { CalciteSchema calciteSchema = schemaCollector.toSchema(rel); CalciteSchema schema1 = calciteSchema.getSubSchema("schema1", false); - hasTable(schema1, "table1", "RecordType(BOOLEAN NOT NULL col1)"); + hasTable(schema1, "table1", "RecordType(BOOLEAN NOT NULL col1) NOT NULL"); - hasTable(schema1, "table2", "RecordType(BOOLEAN NOT NULL col1)"); + hasTable(schema1, "table2", "RecordType(BOOLEAN NOT NULL col1) NOT NULL"); } @Test @@ -146,6 +150,26 @@ void canHandleMultipleSchemas() { hasTable(level2b, "t2", "RecordType(INTEGER col2) NOT NULL"); } + /** + * A nullable schema struct still gives a NOT NULL row type, so Calcite widens the columns on the + * null-generating side of a LEFT join as it would for any other table. + */ + @Test + void aNullableSchemaStructStillWidensUnderAnOuterJoin() { + Rel left = sb.namedScan(List.of("l"), List.of("a"), List.of(R.I32)); + Rel right = + NamedScan.builder() + .names(List.of("r")) + .initialSchema(NamedStruct.of(List.of("b"), N.struct(R.I32))) + .build(); + Rel join = sb.join(input -> sb.bool(true), Join.JoinType.LEFT, left, right); + + RelNode relNode = substraitToCalcite.convert(join); + assertEquals( + "RecordType(INTEGER NOT NULL a, INTEGER b) NOT NULL", + relNode.getRowType().getFullTypeString()); + } + @Test void canHandleDuplicateNamedScans() { Rel table = sb.namedScan(List.of("table"), List.of("col1"), List.of(N.BOOLEAN)); From 6db59511f2835ac7e6ca2f470d5910a8a2911dca Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 29 Sep 2026 11:40:08 +0300 Subject: [PATCH 3/3] fix(isthmus): compare a hint's columns without nested names or nullability describesColumnsOf compared a struct column's whole type, nested names and field nullability included. The declared type converts without a name list, so a column holding a struct never matched and the hint's names were dropped. Now that a nullable struct keeps its fields' nullability, a struct computed by an if_then lost its names the same way. Types are now compared ignoring names and nullability at every depth, through structs, lists and maps. Also restates the comments that described the removed widening, adds full round trips to the nullable struct virtual-table tests, and pins a nested struct in CalciteTypeTest. --- .../isthmus/SubstraitRelNodeConverter.java | 62 +++++++++++++++---- .../isthmus/SubstraitRelVisitor.java | 6 +- .../isthmus/expression/CallConverters.java | 4 +- .../isthmus/expression/LiteralConverter.java | 5 +- .../io/substrait/isthmus/CalciteTypeTest.java | 20 ++++++ .../isthmus/NestedExpressionsTest.java | 7 ++- .../io/substrait/isthmus/OutputNamesTest.java | 25 ++++++++ .../isthmus/VirtualTableScanTest.java | 60 ++++++++++++++++-- 8 files changed, 160 insertions(+), 29 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index 72d319364..3b5b5a161 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -945,12 +945,12 @@ public RelNode visit(VirtualTableScan virtualTableScan, Context context) { * *

Names are what a row, list or map value is rebuilt for: a row's field types are checked * against the schema when the {@link VirtualTableScan} is built, so a value that is one of those - * agrees with its declared type on everything a Substrait type says. It can still be stamped - * nullable where it was not -- Calcite makes a struct's fields nullable along with the struct, - * and a value that cannot be null stands in a column that can. A value that cannot take the - * declared type at all -- a call returning a struct, say, whose names are not the declared ones - * -- is reported here: {@link LogicalProject} and {@link LogicalValues} check the row type they - * are handed with an {@code assert}, which says nothing at all unless assertions are on. + * agrees with its declared type on everything a Substrait type says. Its nullability can still + * differ, since Calcite derives that from the expression: a value that cannot be null stands in a + * column that can. A value that cannot take the declared type at all -- a call returning a + * struct, say, whose names are not the declared ones -- is reported here: {@link LogicalProject} + * and {@link LogicalValues} check the row type they are handed with an {@code assert}, which says + * nothing at all unless assertions are on. * * @param value the converted row value * @param declaredType the type its column is declared at @@ -971,10 +971,11 @@ private RexNode valueAsDeclared(RexNode value, RelDataType declaredType) { /** * Whether a converted value can stand at the type it is declared at. * - *

A value that cannot be null stands in a column that can: Calcite makes a struct's fields - * nullable along with the struct, so the fields of a nullable struct are declared that way - * whatever the values in them are. The other direction is a mismatch, and so is any difference - * beyond nullability -- a field name included, which is the one this rename is about. + *

A value that cannot be null stands in a column that can: Calcite derives a value's + * nullability from its expression, and a value that is never null is valid wherever null is + * allowed. The other direction is a mismatch -- a scalar subquery, say, which Calcite types + * nullable whatever its Substrait type says -- and so is any difference beyond nullability, a + * field name included, which is the one this rename is about. */ private boolean fitsDeclared(RelDataType valueType, RelDataType declaredType) { return SqlTypeUtil.equalSansNullability(typeFactory, valueType, declaredType) @@ -1286,15 +1287,50 @@ protected RelNode applyOutputNames(RelNode relNode, Rel rel, RelNode... inputs) */ private boolean describesColumnsOf(List fields, RelDataType rowType) { for (int field = 0; field < fields.size(); field++) { - RelDataType declared = typeConverter.toCalcite(typeFactory, fields.get(field)); - if (!SqlTypeUtil.equalSansNullability( - declared, rowType.getFieldList().get(field).getType())) { + if (!sameSansNamesAndNullability( + typeConverter.toCalcite(typeFactory, fields.get(field)), + rowType.getFieldList().get(field).getType())) { return false; } } return true; } + /** + * Returns whether two types are the same, ignoring nullability and field names at every depth. + * + *

A column's nullability, and that of anything inside it, is Calcite's to derive from the + * expression producing it. The names inside a column are not what a hint restates, and a declared + * type converts without a name list, so the names inside it are placeholders. Calcite's own + * {@link SqlTypeUtil#equalAsStructSansNullability} ignores both only on the outermost struct. + */ + private static boolean sameSansNamesAndNullability(RelDataType declared, RelDataType actual) { + if (declared.isStruct() || actual.isStruct()) { + if (!declared.isStruct() + || !actual.isStruct() + || declared.getFieldCount() != actual.getFieldCount()) { + return false; + } + for (int field = 0; field < declared.getFieldCount(); field++) { + if (!sameSansNamesAndNullability( + declared.getFieldList().get(field).getType(), + actual.getFieldList().get(field).getType())) { + return false; + } + } + return true; + } + if (declared.getKeyType() != null && actual.getKeyType() != null) { + return sameSansNamesAndNullability(declared.getKeyType(), actual.getKeyType()) + && sameSansNamesAndNullability(declared.getValueType(), actual.getValueType()); + } + if (declared.getComponentType() != null && actual.getComponentType() != null) { + return declared.getSqlTypeName() == actual.getSqlTypeName() + && sameSansNamesAndNullability(declared.getComponentType(), actual.getComponentType()); + } + return SqlTypeUtil.equalSansNullability(declared, actual); + } + /** * Applies an optional field remap to the given node. * diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index 479b6fb8f..6178613a4 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -982,9 +982,9 @@ public Rel handleCreateView(CreateView createView) { @Override public Rel visit(VirtualTable virtualTable) { // At the row type's field types rather than the values' own, as visit(Values) does: a literal - // narrower than its column -- Calcite infers one for a tuple value, and pushes a struct's - // nullability down into its fields -- would otherwise disagree with the schema built from the - // same row type, and VirtualTableScan rejects the relation on that. + // narrower than its column -- Calcite infers one for a tuple value -- would otherwise disagree + // with the schema built from the same row type, and VirtualTableScan rejects the relation on + // that. List rowFields = virtualTable.getRowType().getFieldList(); LiteralConverter literalConverter = new LiteralConverter(typeConverter); List rows = new ArrayList<>(virtualTable.getRows().size()); diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/CallConverters.java b/isthmus/src/main/java/io/substrait/isthmus/expression/CallConverters.java index c35556103..4cc78103d 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/CallConverters.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/CallConverters.java @@ -154,8 +154,8 @@ else if (operand instanceof Expression.StructLiteral * that: it builds a deliberately non-nullable ROW, keeping the user-defined type's own * nullability in the REINTERPRET target type, and so still arrives here as a StructLiteral. * - *

Each literal's nullability is set to match its field type's nullability. Note that Calcite - * makes every field of a nullable record type nullable, so a nullable ROW widens its fields. + *

Each literal's nullability is set to match its field type's nullability, which the ROW's + * type records field by field, whatever the ROW's own nullability is. */ public static final SimpleCallConverter ROW = (call, visitor) -> { diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java index ebd14d978..65b84f74f 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java @@ -196,8 +196,9 @@ public Expression.Literal convert(RexLiteral literal) { *

This overload is useful when the target type comes from a containing schema rather than the * literal itself. Calcite may infer a narrower type for a value in a LogicalValues tuple than for * the corresponding row field. Nullability is taken from {@code resultType}, so callers that need - * a nullability other than the literal's own should widen the Calcite type with {@link - * org.apache.calcite.rel.type.RelDataTypeFactory#createTypeWithNullability} before calling. + * a nullability other than the literal's own should set it on the Calcite type with {@link + * org.apache.calcite.rel.type.RelDataTypeFactory#enforceTypeWithNullability} before calling: + * {@code createTypeWithNullability} would make a struct's fields nullable along with it. * * @param literal the RexLiteral to convert * @param resultType the Calcite type required by the containing schema diff --git a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java index 24b5ee2e1..56d3ea749 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java @@ -339,6 +339,26 @@ void aNullableStructKeepsWhatItsFieldsDeclare() { assertEquals(substrait, TypeConverter.DEFAULT.toSubstrait(calcite)); } + /** + * The widening this replaces was recursive, so a struct below the outermost one is pinned too: a + * nullable struct holding a required struct keeps the inner struct's own nullability as well as + * its fields'. + */ + @Test + void aNestedStructKeepsWhatItsFieldsDeclare() { + Type.Struct substrait = + TypeCreator.NULLABLE.struct( + TypeCreator.REQUIRED.struct(TypeCreator.REQUIRED.I32, TypeCreator.NULLABLE.FP64)); + + RelDataType calcite = + TypeConverter.DEFAULT.toCalcite(type, substrait, List.of("inner", "a", "b")); + + assertEquals( + "RecordType(RecordType(INTEGER NOT NULL a, DOUBLE b) NOT NULL inner)", + calcite.getFullTypeString()); + assertEquals(substrait, TypeConverter.DEFAULT.toSubstrait(calcite)); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) void userDefinedType(boolean nullable) { diff --git a/isthmus/src/test/java/io/substrait/isthmus/NestedExpressionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/NestedExpressionsTest.java index bc693e809..5144c9875 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/NestedExpressionsTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/NestedExpressionsTest.java @@ -273,10 +273,11 @@ void nullableNestedStructWithLiteralsTest() { @Test void nullableStructLiteralTest() { - // The same nullability, on a value that is a StructLiteral to begin with. Its fields are - // nullable because Calcite makes every field of a nullable record type nullable. + // The same nullability, on a value that is a StructLiteral to begin with. Each field keeps its + // own nullability, whatever the struct's is. Expression.StructLiteral structLiteral = - ExpressionCreator.struct(true, ExpressionCreator.i32(true, 7)); + ExpressionCreator.struct( + true, ExpressionCreator.i32(true, 7), ExpressionCreator.i32(false, 8)); Project project = Project.builder().expressions(List.of(structLiteral)).input(emptyTable).build(); diff --git a/isthmus/src/test/java/io/substrait/isthmus/OutputNamesTest.java b/isthmus/src/test/java/io/substrait/isthmus/OutputNamesTest.java index a8f380022..f7c2b50dc 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/OutputNamesTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/OutputNamesTest.java @@ -93,6 +93,31 @@ private Project structColumnProject(Rel structScan, Optional hint) { return builder.build(); } + @Test + void namesAStructColumnComputedByAnIfThen() { + // Calcite types a CASE over a nullable struct with nullable fields, where the relation declares + // the struct's first field required. That field's nullability is the expression's to derive, + // like the column's own, so it does not keep the names off the column. + Type.Struct inner = TypeCreator.NULLABLE.struct(R.I64, N.STRING); + Rel structScan = sb.namedScan(List.of("t"), List.of("s", "x", "y", "k"), List.of(inner, R.I64)); + Rel project = + Project.builder() + .input(structScan) + .remap(Rel.Remap.offset(2, 1)) + .addExpressions( + sb.ifThen( + List.of( + sb.ifClause( + sb.equal(sb.fieldReference(structScan, 1), sb.i64(1)), + sb.fieldReference(structScan, 0))), + sb.fieldReference(structScan, 0))) + .hint(Hint.builder().addOutputNames("renamed", "first", "second").build()) + .build(); + + assertEquals( + List.of("renamed"), substraitToCalcite.convert(project).getRowType().getFieldNames()); + } + @Test void keepsCalciteNamesWithoutAHint() { RelNode node = substraitToCalcite.convert(projectWithHint(Optional.empty())); diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java index 989793e99..7f16cc275 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java @@ -14,6 +14,7 @@ import io.substrait.relation.Rel; import io.substrait.relation.VirtualTableScan; import io.substrait.type.NamedStruct; +import io.substrait.type.Type; import java.io.PrintWriter; import java.io.StringWriter; import java.math.BigDecimal; @@ -273,9 +274,8 @@ void listColumnConverts() { /** * Calcite has no nullable literal of a row, so a struct value in a nullable column cannot be a - * Values tuple whatever it holds, and the row takes the projection encoding. Pinned as a - * conversion rather than a round trip because a nullable struct does not survive one: Calcite - * pushes a row's nullability down into its fields, so the schema comes back with nullable fields. + * Values tuple whatever it holds, and the row takes the projection encoding. The fields keep the + * nullability the schema declares, so the table comes back as it was. */ @Test void nullableStructColumnConverts() { @@ -288,6 +288,8 @@ void nullableStructColumnConverts() { assertEquals( "VirtualTable(type=[RecordType(RecordType(INTEGER a, DOUBLE b) outer)], rows=[[{ ROW(1, 2.0E0:DOUBLE) }]])\n", explain(relNode)); + + assertFullRoundTrip(virtualTableScan); } /** A null struct has no fields to rename, but the tuple still needs it at the column's type. */ @@ -418,13 +420,13 @@ void aComputedFieldInsideANullableStructConvertsBack() { assertEquals( schema.struct(), SubstraitRelVisitor.convert(relNode, converterProvider).getRecordType()); + assertFullRoundTrip(virtualTableScan); } /** * A nullable struct nested in a column: renaming it gives back a ROW call rather than a literal, - * so the struct around it cannot be rebuilt as a literal either. Pinned as a conversion rather - * than a round trip for the same reason as its sibling above: Calcite pushes a struct's - * nullability down into its fields, so the schema comes back with nullable fields. + * so the struct around it cannot be rebuilt as a literal either. As for its sibling above, the + * fields keep their declared nullability and the table comes back as it was. */ @Test void nullableStructInsideStructColumnConverts() { @@ -439,6 +441,8 @@ void nullableStructInsideStructColumnConverts() { assertEquals( "VirtualTable(type=[RecordType(RecordType(RecordType(INTEGER a) inner) outer)], rows=[[{ ROW(ROW(1)) }]])\n", explain(relNode)); + + assertFullRoundTrip(virtualTableScan); } /** @@ -584,6 +588,50 @@ void outputNamesReachTheProjectionTheMappingAddsOnAComputedTable() { List.of(R.FP64), SubstraitRelVisitor.convert(relNode, extensions).getRecordType().fields()); } + /** + * The same for a column holding a struct, at any depth. Only the column's own name is restated, + * so the names inside it are not compared: the declared type converts without them. + */ + @Test + void outputNamesReachAColumnHoldingAStruct() { + Expression.Literal struct = ExpressionCreator.struct(false, sb.i32(2), sb.fp64(3.0)); + assertOutputNameReaches(R.struct(R.I32, R.FP64), struct, "a", "b"); + assertOutputNameReaches( + R.struct(N.struct(R.I32), R.FP64), + ExpressionCreator.struct(false, ExpressionCreator.struct(true, sb.i32(2)), sb.fp64(3.0)), + "inner", + "a", + "b"); + assertOutputNameReaches( + R.list(R.struct(R.I32, R.FP64)), ExpressionCreator.list(false, struct), "a", "b"); + assertOutputNameReaches( + R.map(R.STRING, R.struct(R.I32, R.FP64)), + ExpressionCreator.map(false, Map.of(sb.str("k"), struct)), + "a", + "b"); + } + + private void assertOutputNameReaches( + Type column, Expression.Literal value, String... namesInside) { + List names = new ArrayList<>(List.of("plain", "column")); + names.addAll(List.of(namesInside)); + List outputNames = new ArrayList<>(List.of("label")); + outputNames.addAll(List.of(namesInside)); + VirtualTableScan table = + VirtualTableScan.builder() + .from( + virtualTable( + NamedStruct.of(names, R.struct(R.I32, column)), List.of(sb.i32(1), value))) + .remap(Rel.Remap.of(List.of(1))) + .hint(Hint.builder().addAllOutputNames(outputNames).build()) + .build(); + + assertEquals( + List.of("label"), + substraitToCalcite.convert(table).getRowType().getFieldNames(), + column.toString()); + } + private String explain(RelNode relNode) { // Setting DIGEST_ATTRIBUTES in order to verify types in tests StringWriter sw = new StringWriter();