From 1034f1195aa3cab57e881e4b487841d85ad63f7d Mon Sep 17 00:00:00 2001 From: bvolpato Date: Thu, 3 Sep 2026 18:05:40 +0000 Subject: [PATCH 1/7] fix(isthmus)!: translate array indexes correctly Normalize safe array indexes by their operator base. Preserve null and out-of-range low indexes without introducing negative list offsets, and check wide indexes before narrowing. BREAKING CHANGE: Unsafe array operators and unrepresentable offsets now reach unsupported-function handling instead of changing semantics. --- .../expression/FieldSelectionConverter.java | 43 ++++- .../isthmus/NestedStructQueryTest.java | 10 +- .../FieldSelectionConverterTest.java | 166 ++++++++++++++++++ 3 files changed, 209 insertions(+), 10 deletions(-) create mode 100644 isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java index 4893d1ef5..f83d42ceb 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java @@ -12,6 +12,7 @@ import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.fun.SqlItemOperator; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -43,7 +44,7 @@ public FieldSelectionConverter(TypeConverter typeConverter) { * * * @@ -65,7 +66,8 @@ public Optional convert( LOGGER .atWarn() .log( - "Found item operator without literal kind/type. This isn't handled well. Reference was {} with toString {}.", + "Found item operator without literal kind/type. This isn't handled well. Reference" + + " was {} with toString {}.", reference.getKind().name(), reference); return Optional.empty(); @@ -90,15 +92,39 @@ public Optional convert( } case ARRAY: { - Optional index = toInt(literal); + if (!(call.getOperator() instanceof SqlItemOperator)) { + return Optional.empty(); + } + SqlItemOperator operator = (SqlItemOperator) call.getOperator(); + // A Substrait list reference returns null for an out-of-range index, so it cannot + // preserve the error behavior of OFFSET or ORDINAL. + if (!operator.safe || (operator.offset != 0 && operator.offset != 1)) { + return Optional.empty(); + } + if (literal instanceof Expression.NullLiteral) { + return Optional.of( + ExpressionCreator.typedNull(typeConverter.toSubstrait(call.getType()))); + } + + Optional index = toLong(literal); if (index.isEmpty()) { return Optional.empty(); } + // Substrait negative offsets count from the end of the list. Calcite treats an index + // below the operator's base as out of range, including zero for one-based ITEM. + if (index.get() < operator.offset) { + return Optional.of( + ExpressionCreator.typedNull(typeConverter.toSubstrait(call.getType()))); + } + long offset = index.get() - operator.offset; + if (offset > Integer.MAX_VALUE) { + return Optional.empty(); + } if (input instanceof FieldReference) { - return Optional.of(((FieldReference) input).dereferenceList(index.get())); + return Optional.of(((FieldReference) input).dereferenceList((int) offset)); } else { - return Optional.of(FieldReference.newListReference(index.get(), input)); + return Optional.of(FieldReference.newListReference((int) offset, input)); } } @@ -141,6 +167,13 @@ private Optional toInt(Expression.Literal l) { return Optional.empty(); } + private Optional toLong(Expression.Literal literal) { + if (literal instanceof Expression.I64Literal) { + return Optional.of(((Expression.I64Literal) literal).value()); + } + return toInt(literal).map(Integer::longValue); + } + /** * Converts a fixed-char literal to a string key. * diff --git a/isthmus/src/test/java/io/substrait/isthmus/NestedStructQueryTest.java b/isthmus/src/test/java/io/substrait/isthmus/NestedStructQueryTest.java index f1400526a..dbcc66592 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/NestedStructQueryTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/NestedStructQueryTest.java @@ -204,7 +204,7 @@ public RelDataType getRowType(RelDataTypeFactory factory) { + " field: 1 # a\n" + " child {\n" + " list_element {\n" - + " offset: 1\n" + + " offset: 0\n" + " }\n" + " }\n" + " }\n" @@ -244,13 +244,13 @@ public RelDataType getRowType(RelDataTypeFactory factory) { + " field: 1 # a\n" + " child {\n" + " list_element {\n" - + " offset: 1\n" + + " offset: 0\n" + " child {\n" + " list_element {\n" - + " offset: 2\n" + + " offset: 1\n" + " child {\n" + " list_element {\n" - + " offset: 3\n" + + " offset: 2\n" + " }\n" + " }\n" + " }\n" @@ -300,7 +300,7 @@ public RelDataType getRowType(RelDataTypeFactory factory) { + " field: 0 # .b\n" + " child {\n" + " list_element {\n" - + " offset: 2\n" + + " offset: 1\n" + " child {\n" + " struct_field {\n" + " field: 0 # .c\n" diff --git a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java new file mode 100644 index 000000000..7927330a8 --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java @@ -0,0 +1,166 @@ +package io.substrait.isthmus.expression; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import io.substrait.expression.Expression; +import io.substrait.expression.FieldReference; +import io.substrait.expression.proto.ExpressionProtoConverter; +import io.substrait.extension.ExtensionCollector; +import io.substrait.type.TypeCreator; +import io.substrait.util.EmptyVisitationContext; +import java.math.BigDecimal; +import java.util.List; +import java.util.stream.Stream; +import org.apache.calcite.DataContexts; +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexExecutable; +import org.apache.calcite.rex.RexExecutorImpl; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.fun.SqlLibraryOperators; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.type.SqlTypeName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; + +class FieldSelectionConverterTest { + private final JavaTypeFactoryImpl typeFactory = new JavaTypeFactoryImpl(); + private final RexBuilder rexBuilder = new RexBuilder(typeFactory); + private final RelDataType intType = typeFactory.createSqlType(SqlTypeName.INTEGER); + private final RexExpressionConverter converter = new RexExpressionConverter(); + + @ParameterizedTest + @CsvSource({"1, 0, 10", "2, 1, 20", "3, 2, 30", "4, 3,", "0,,", "-1,,", "-2,,"}) + void itemPreservesCalciteIndexing(int index, Integer expectedOffset, Integer expectedValue) { + RexNode call = rexBuilder.makeCall(SqlStdOperatorTable.ITEM, array(), integer(index)); + RexExecutable executable = + RexExecutorImpl.getExecutable(rexBuilder, List.of(call), typeFactory.builder().build()); + executable.setDataContext(DataContexts.EMPTY); + assertEquals(expectedValue, executable.execute()[0]); + + Expression converted = call.accept(converter); + if (expectedOffset == null) { + assertEquals( + TypeCreator.NULLABLE.I32, + assertInstanceOf(Expression.NullLiteral.class, converted).getType()); + } else { + assertEquals(expectedOffset.intValue(), listOffset(converted)); + } + } + + @ParameterizedTest + @MethodSource("safeIndexing") + void safeOperatorsUseTheirOwnBase(SqlOperator operator, long index, int expectedOffset) { + Expression expression = + rexBuilder.makeCall(operator, array(), integer(index)).accept(converter); + assertEquals(expectedOffset, listOffset(expression)); + + // The same offset is used when dereferencing an array column rather than an expression. + RexNode column = rexBuilder.makeInputRef(array().getType(), 0); + Expression columnExpression = + rexBuilder.makeCall(operator, column, integer(index)).accept(converter); + io.substrait.proto.Expression proto = toProto(columnExpression); + assertEquals( + expectedOffset, + proto + .getSelection() + .getDirectReference() + .getStructField() + .getChild() + .getListElement() + .getOffset()); + } + + private static Stream safeIndexing() { + return Stream.of( + Arguments.of(SqlStdOperatorTable.ITEM, 1L, 0), + Arguments.of(SqlStdOperatorTable.ITEM, 2147483648L, Integer.MAX_VALUE), + Arguments.of(SqlLibraryOperators.SAFE_OFFSET, 0L, 0), + Arguments.of(SqlLibraryOperators.SAFE_OFFSET, 1L, 1), + Arguments.of(SqlLibraryOperators.SAFE_OFFSET, 3L, 3), + Arguments.of(SqlLibraryOperators.SAFE_OFFSET, (long) Integer.MAX_VALUE, Integer.MAX_VALUE), + Arguments.of(SqlLibraryOperators.SAFE_ORDINAL, 1L, 0), + Arguments.of(SqlLibraryOperators.SAFE_ORDINAL, 3L, 2), + Arguments.of(SqlLibraryOperators.SAFE_ORDINAL, 2147483648L, Integer.MAX_VALUE)); + } + + @ParameterizedTest + @MethodSource("invalidLowIndexes") + void invalidLowIndexesAreNull(SqlOperator operator, long index) { + Expression converted = rexBuilder.makeCall(operator, array(), integer(index)).accept(converter); + assertEquals( + TypeCreator.NULLABLE.I32, + assertInstanceOf(Expression.NullLiteral.class, converted).getType()); + } + + private static Stream invalidLowIndexes() { + return Stream.of( + Arguments.of(SqlStdOperatorTable.ITEM, 0L), + Arguments.of(SqlStdOperatorTable.ITEM, Long.MIN_VALUE), + Arguments.of(SqlLibraryOperators.SAFE_OFFSET, -1L), + Arguments.of(SqlLibraryOperators.SAFE_OFFSET, Long.MIN_VALUE), + Arguments.of(SqlLibraryOperators.SAFE_ORDINAL, 0L), + Arguments.of(SqlLibraryOperators.SAFE_ORDINAL, -1L)); + } + + @Test + void nullIndexIsNull() { + Expression converted = + rexBuilder + .makeCall(SqlStdOperatorTable.ITEM, array(), rexBuilder.makeNullLiteral(intType)) + .accept(converter); + assertEquals( + TypeCreator.NULLABLE.I32, + assertInstanceOf(Expression.NullLiteral.class, converted).getType()); + } + + @ParameterizedTest + @ValueSource(longs = {2147483649L, 4294967297L, Long.MAX_VALUE}) + void unrepresentableOffsetsAreRejected(long index) { + RexNode call = rexBuilder.makeCall(SqlStdOperatorTable.ITEM, array(), integer(index)); + assertThrows(IllegalArgumentException.class, () -> call.accept(converter)); + } + + @ParameterizedTest + @MethodSource("unsafeOperators") + void throwingOperatorsAreRejected(SqlOperator operator) { + RexNode call = rexBuilder.makeCall(operator, array(), integer(4)); + assertThrows(IllegalArgumentException.class, () -> call.accept(converter)); + } + + private static Stream unsafeOperators() { + return Stream.of(SqlLibraryOperators.OFFSET, SqlLibraryOperators.ORDINAL); + } + + private RexNode array() { + return rexBuilder.makeCall( + SqlStdOperatorTable.ARRAY_VALUE_CONSTRUCTOR, integer(10), integer(20), integer(30)); + } + + private RexNode integer(long value) { + RelDataType type = + value >= Integer.MIN_VALUE && value <= Integer.MAX_VALUE + ? intType + : typeFactory.createSqlType(SqlTypeName.BIGINT); + return rexBuilder.makeExactLiteral(BigDecimal.valueOf(value), type); + } + + private int listOffset(Expression expression) { + assertInstanceOf(FieldReference.class, expression); + return toProto(expression).getSelection().getDirectReference().getListElement().getOffset(); + } + + private io.substrait.proto.Expression toProto(Expression expression) { + return expression.accept( + new ExpressionProtoConverter(new ExtensionCollector(), null), + EmptyVisitationContext.INSTANCE); + } +} From bafd921a1217b66842fdab86d309b871e4a2d93f Mon Sep 17 00:00:00 2001 From: bvolpato Date: Thu, 3 Sep 2026 19:58:49 +0000 Subject: [PATCH 2/7] fix(isthmus): preserve array operand evaluation for invalid indexes Folding an invalid or null index to NULL can remove a throwing cast or another computed array operand. Restrict this fold to literals and references into an existing record; reject other operands when their evaluation cannot be preserved. Cover throwing casts, nested array selections, computed operands, and safe literal and column references for ITEM, SAFE_OFFSET, and SAFE_ORDINAL. --- .../expression/FieldSelectionConverter.java | 17 ++++-- .../FieldSelectionConverterTest.java | 60 +++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java index f83d42ceb..146eafa34 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java @@ -102,8 +102,7 @@ public Optional convert( return Optional.empty(); } if (literal instanceof Expression.NullLiteral) { - return Optional.of( - ExpressionCreator.typedNull(typeConverter.toSubstrait(call.getType()))); + return nullIfInputCanBeDiscarded(call, input); } Optional index = toLong(literal); @@ -113,8 +112,7 @@ public Optional convert( // Substrait negative offsets count from the end of the list. Calcite treats an index // below the operator's base as out of range, including zero for one-based ITEM. if (index.get() < operator.offset) { - return Optional.of( - ExpressionCreator.typedNull(typeConverter.toSubstrait(call.getType()))); + return nullIfInputCanBeDiscarded(call, input); } long offset = index.get() - operator.offset; if (offset > Integer.MAX_VALUE) { @@ -147,6 +145,17 @@ public Optional convert( return Optional.empty(); } + private Optional nullIfInputCanBeDiscarded(RexCall call, Expression input) { + // Calcite still evaluates the array operand for a null or out-of-range index. Only literals + // and references into an existing record are safe to omit; even deterministic calls can fail. + if (input instanceof Literal + || (input instanceof FieldReference + && ((FieldReference) input).inputExpression().isEmpty())) { + return Optional.of(ExpressionCreator.typedNull(typeConverter.toSubstrait(call.getType()))); + } + return Optional.empty(); + } + /** * Converts a numeric literal to an integer index. * diff --git a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java index 7927330a8..bbb89a6a6 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java @@ -122,6 +122,66 @@ void nullIndexIsNull() { assertInstanceOf(Expression.NullLiteral.class, converted).getType()); } + @ParameterizedTest + @MethodSource("nullOrInvalidIndexes") + void rejectsDiscardingThrowingArray(SqlOperator operator, Integer index) { + RexNode cast = + rexBuilder.makeAbstractCast(intType, rexBuilder.makeLiteral("not an integer"), false); + RexNode throwingArray = rexBuilder.makeCall(SqlStdOperatorTable.ARRAY_VALUE_CONSTRUCTOR, cast); + RexNode nestedArray = + rexBuilder.makeCall(SqlStdOperatorTable.ARRAY_VALUE_CONSTRUCTOR, throwingArray); + RexNode nestedSelection = + rexBuilder.makeCall(SqlStdOperatorTable.ITEM, nestedArray, integer(1)); + + for (RexNode input : List.of(throwingArray, nestedSelection)) { + RexNode call = rexBuilder.makeCall(operator, input, nullableIndex(index)); + RexExecutable executable = + RexExecutorImpl.getExecutable(rexBuilder, List.of(call), typeFactory.builder().build()); + executable.setDataContext(DataContexts.EMPTY); + assertThrows(NumberFormatException.class, executable::execute); + assertThrows(IllegalArgumentException.class, () -> call.accept(converter)); + } + } + + @ParameterizedTest + @MethodSource("nullOrInvalidIndexes") + void rejectsDiscardingNonliteralArray(SqlOperator operator, Integer index) { + RexNode text = rexBuilder.makeInputRef(typeFactory.createSqlType(SqlTypeName.VARCHAR, 20), 0); + RexNode cast = rexBuilder.makeAbstractCast(intType, text, false); + RexNode array = rexBuilder.makeCall(SqlStdOperatorTable.ARRAY_VALUE_CONSTRUCTOR, cast); + RexNode call = rexBuilder.makeCall(operator, array, nullableIndex(index)); + + assertThrows(IllegalArgumentException.class, () -> call.accept(converter)); + } + + @ParameterizedTest + @MethodSource("nullOrInvalidIndexes") + void literalAndColumnArraysCanBeDiscarded(SqlOperator operator, Integer index) { + RexNode column = rexBuilder.makeInputRef(array().getType(), 0); + for (RexNode input : List.of(array(), column)) { + Expression converted = + rexBuilder.makeCall(operator, input, nullableIndex(index)).accept(converter); + assertEquals( + TypeCreator.NULLABLE.I32, + assertInstanceOf(Expression.NullLiteral.class, converted).getType()); + } + } + + private static Stream nullOrInvalidIndexes() { + return Stream.of( + Arguments.of(SqlStdOperatorTable.ITEM, 0), + Arguments.of(SqlStdOperatorTable.ITEM, -1), + Arguments.of(SqlStdOperatorTable.ITEM, null), + Arguments.of(SqlLibraryOperators.SAFE_OFFSET, -1), + Arguments.of(SqlLibraryOperators.SAFE_OFFSET, null), + Arguments.of(SqlLibraryOperators.SAFE_ORDINAL, 0), + Arguments.of(SqlLibraryOperators.SAFE_ORDINAL, null)); + } + + private RexNode nullableIndex(Integer index) { + return index == null ? rexBuilder.makeNullLiteral(intType) : integer(index); + } + @ParameterizedTest @ValueSource(longs = {2147483649L, 4294967297L, Long.MAX_VALUE}) void unrepresentableOffsetsAreRejected(long index) { From 3424c923a31ba32b31f0cc61964520ef065401df Mon Sep 17 00:00:00 2001 From: Bruno Volpato Date: Sat, 3 Oct 2026 17:53:46 -0400 Subject: [PATCH 3/7] fix(isthmus)!: retain invalid array selections with nullable types Use a guaranteed out-of-range Substrait list offset for null and below-base indexes, retaining computed operand evaluation and collection composition. Infer nullable list-selection results consistently through nested references and protobuf conversion. BREAKING CHANGE: List selections now report nullable element types, and selections through nullable struct or map parents propagate that nullability. --- .../substrait/expression/FieldReference.java | 9 +- .../proto/FieldReferenceRoundtripTest.java | 40 +++++ .../expression/FieldSelectionConverter.java | 37 ++--- .../FieldSelectionConverterTest.java | 144 +++++++++++++++--- 4 files changed, 183 insertions(+), 47 deletions(-) diff --git a/core/src/main/java/io/substrait/expression/FieldReference.java b/core/src/main/java/io/substrait/expression/FieldReference.java index 92e726fe3..796e2ba05 100644 --- a/core/src/main/java/io/substrait/expression/FieldReference.java +++ b/core/src/main/java/io/substrait/expression/FieldReference.java @@ -532,7 +532,8 @@ public Type visit(Type.Struct expr) throws RuntimeException { if (expr.fields().size() < index) { throw new IllegalArgumentException("Undefined struct type."); } - return expr.fields().get(index); + Type field = expr.fields().get(index); + return field.withNullable(field.nullable() || expr.nullable()); } public static Type getReferencedType(Type type, int index) { @@ -549,7 +550,8 @@ private ListIndexFinder() { @Override public Type visit(Type.ListType expr) throws RuntimeException { - return expr.elementType(); + // An out-of-range offset returns null even when the list's elements are required. + return expr.elementType().withNullable(true); } public static Type getReferencedType(Type type, int index) { @@ -575,7 +577,8 @@ public Type visit(Type.Map expr) throws RuntimeException { String.format( "Key type %s of map does not matched expected type of %s.", expr.key(), keyType)); } - return expr.value(); + Type value = expr.value(); + return value.withNullable(value.nullable() || expr.nullable()); } public static Type getReferencedType(Type typeToDereference, Type keyType) { diff --git a/core/src/test/java/io/substrait/type/proto/FieldReferenceRoundtripTest.java b/core/src/test/java/io/substrait/type/proto/FieldReferenceRoundtripTest.java index 4e00797c2..71243a396 100644 --- a/core/src/test/java/io/substrait/type/proto/FieldReferenceRoundtripTest.java +++ b/core/src/test/java/io/substrait/type/proto/FieldReferenceRoundtripTest.java @@ -1,13 +1,18 @@ package io.substrait.type.proto; +import static org.junit.jupiter.api.Assertions.assertEquals; + import io.substrait.TestBase; import io.substrait.expression.Expression; +import io.substrait.expression.ExpressionCreator; +import io.substrait.expression.FieldReference; import io.substrait.relation.Filter; import io.substrait.relation.Project; import io.substrait.relation.Rel; import io.substrait.type.Type; import java.util.Arrays; import java.util.Collections; +import java.util.List; import org.junit.jupiter.api.Test; /** @@ -158,4 +163,39 @@ void sameFieldReferencedMultipleTimes() { verifyRoundTrip(projection); } + + @Test + void listSelectionsRemainNullableThroughNestedFields() { + Rel input = + sb.namedScan( + List.of("arrays"), + List.of("lists", "rows", "maps"), + List.of( + R.list(R.list(R.I32)), R.list(R.struct(R.I32)), R.list(R.map(R.STRING, R.I32)))); + for (int offset : new int[] {0, -1, Integer.MAX_VALUE}) { + List selections = + List.of( + FieldReference.newInputRelReference(0, input) + .dereferenceList(offset) + .dereferenceList(0), + FieldReference.newInputRelReference(1, input) + .dereferenceList(offset) + .dereferenceStruct(0), + FieldReference.newInputRelReference(2, input) + .dereferenceList(offset) + .dereferenceMap(ExpressionCreator.string(false, "key"))); + selections.forEach(selection -> assertEquals(N.I32, selection.getType())); + verifyRoundTrip(Project.builder().input(input).expressions(selections).build()); + } + } + + @Test + void listSelectionOnExpressionRemainsNullable() { + Expression input = ExpressionCreator.list(false, ExpressionCreator.i32(false, 1)); + for (int offset : new int[] {0, Integer.MAX_VALUE}) { + FieldReference selection = FieldReference.newListReference(offset, input); + assertEquals(N.I32, selection.getType()); + verifyRoundTrip(selection); + } + } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java index 146eafa34..048f18c91 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java @@ -101,20 +101,20 @@ public Optional convert( if (!operator.safe || (operator.offset != 0 && operator.offset != 1)) { return Optional.empty(); } - if (literal instanceof Expression.NullLiteral) { - return nullIfInputCanBeDiscarded(call, input); + long offset = Integer.MAX_VALUE; + if (!(literal instanceof Expression.NullLiteral)) { + Optional index = toLong(literal); + if (index.isEmpty()) { + return Optional.empty(); + } + // Negative Substrait offsets count from the end, whereas Calcite returns null below + // the operator's base. Substrait lists have at most Integer.MAX_VALUE elements, so that + // zero-based offset cannot select an element. Keep the reference to evaluate the + // array operand and preserve its element type, including for a null index. + if (index.get() >= operator.offset) { + offset = index.get() - operator.offset; + } } - - Optional index = toLong(literal); - if (index.isEmpty()) { - return Optional.empty(); - } - // Substrait negative offsets count from the end of the list. Calcite treats an index - // below the operator's base as out of range, including zero for one-based ITEM. - if (index.get() < operator.offset) { - return nullIfInputCanBeDiscarded(call, input); - } - long offset = index.get() - operator.offset; if (offset > Integer.MAX_VALUE) { return Optional.empty(); } @@ -145,17 +145,6 @@ public Optional convert( return Optional.empty(); } - private Optional nullIfInputCanBeDiscarded(RexCall call, Expression input) { - // Calcite still evaluates the array operand for a null or out-of-range index. Only literals - // and references into an existing record are safe to omit; even deterministic calls can fail. - if (input instanceof Literal - || (input instanceof FieldReference - && ((FieldReference) input).inputExpression().isEmpty())) { - return Optional.of(ExpressionCreator.typedNull(typeConverter.toSubstrait(call.getType()))); - } - return Optional.empty(); - } - /** * Converts a numeric literal to an integer index. * diff --git a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java index bbb89a6a6..ab52d4ad6 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java @@ -7,7 +7,13 @@ import io.substrait.expression.Expression; import io.substrait.expression.FieldReference; import io.substrait.expression.proto.ExpressionProtoConverter; +import io.substrait.expression.proto.ProtoExpressionConverter; +import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.ExtensionCollector; +import io.substrait.isthmus.TypeConverter; +import io.substrait.isthmus.UserTypeMapper; +import io.substrait.isthmus.utils.UserTypeFactory; +import io.substrait.type.Type; import io.substrait.type.TypeCreator; import io.substrait.util.EmptyVisitationContext; import java.math.BigDecimal; @@ -47,13 +53,9 @@ void itemPreservesCalciteIndexing(int index, Integer expectedOffset, Integer exp assertEquals(expectedValue, executable.execute()[0]); Expression converted = call.accept(converter); - if (expectedOffset == null) { - assertEquals( - TypeCreator.NULLABLE.I32, - assertInstanceOf(Expression.NullLiteral.class, converted).getType()); - } else { - assertEquals(expectedOffset.intValue(), listOffset(converted)); - } + assertEquals( + expectedOffset == null ? Integer.MAX_VALUE : expectedOffset, listOffset(converted)); + assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); } @ParameterizedTest @@ -96,9 +98,8 @@ private static Stream safeIndexing() { @MethodSource("invalidLowIndexes") void invalidLowIndexesAreNull(SqlOperator operator, long index) { Expression converted = rexBuilder.makeCall(operator, array(), integer(index)).accept(converter); - assertEquals( - TypeCreator.NULLABLE.I32, - assertInstanceOf(Expression.NullLiteral.class, converted).getType()); + assertEquals(Integer.MAX_VALUE, listOffset(converted)); + assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); } private static Stream invalidLowIndexes() { @@ -117,14 +118,13 @@ void nullIndexIsNull() { rexBuilder .makeCall(SqlStdOperatorTable.ITEM, array(), rexBuilder.makeNullLiteral(intType)) .accept(converter); - assertEquals( - TypeCreator.NULLABLE.I32, - assertInstanceOf(Expression.NullLiteral.class, converted).getType()); + assertEquals(Integer.MAX_VALUE, listOffset(converted)); + assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); } @ParameterizedTest @MethodSource("nullOrInvalidIndexes") - void rejectsDiscardingThrowingArray(SqlOperator operator, Integer index) { + void retainsThrowingArray(SqlOperator operator, Integer index) { RexNode cast = rexBuilder.makeAbstractCast(intType, rexBuilder.makeLiteral("not an integer"), false); RexNode throwingArray = rexBuilder.makeCall(SqlStdOperatorTable.ARRAY_VALUE_CONSTRUCTOR, cast); @@ -139,31 +139,43 @@ void rejectsDiscardingThrowingArray(SqlOperator operator, Integer index) { RexExecutorImpl.getExecutable(rexBuilder, List.of(call), typeFactory.builder().build()); executable.setDataContext(DataContexts.EMPTY); assertThrows(NumberFormatException.class, executable::execute); - assertThrows(IllegalArgumentException.class, () -> call.accept(converter)); + FieldReference converted = assertInstanceOf(FieldReference.class, call.accept(converter)); + assertEquals( + Integer.MAX_VALUE, ((FieldReference.ListElement) converted.segments().get(0)).offset()); + Expression source = input.accept(converter); + assertEquals( + source instanceof FieldReference + ? ((FieldReference) source).inputExpression().orElseThrow() + : source, + converted.inputExpression().orElseThrow()); } } @ParameterizedTest @MethodSource("nullOrInvalidIndexes") - void rejectsDiscardingNonliteralArray(SqlOperator operator, Integer index) { + void retainsNonliteralArray(SqlOperator operator, Integer index) { RexNode text = rexBuilder.makeInputRef(typeFactory.createSqlType(SqlTypeName.VARCHAR, 20), 0); RexNode cast = rexBuilder.makeAbstractCast(intType, text, false); RexNode array = rexBuilder.makeCall(SqlStdOperatorTable.ARRAY_VALUE_CONSTRUCTOR, cast); RexNode call = rexBuilder.makeCall(operator, array, nullableIndex(index)); - assertThrows(IllegalArgumentException.class, () -> call.accept(converter)); + FieldReference converted = assertInstanceOf(FieldReference.class, call.accept(converter)); + assertEquals(Integer.MAX_VALUE, listOffset(converted)); + assertEquals(array.accept(converter), converted.inputExpression().orElseThrow()); } @ParameterizedTest @MethodSource("nullOrInvalidIndexes") - void literalAndColumnArraysCanBeDiscarded(SqlOperator operator, Integer index) { + void literalAndColumnArraysRetainReferences(SqlOperator operator, Integer index) { RexNode column = rexBuilder.makeInputRef(array().getType(), 0); for (RexNode input : List.of(array(), column)) { Expression converted = rexBuilder.makeCall(operator, input, nullableIndex(index)).accept(converter); + FieldReference reference = assertInstanceOf(FieldReference.class, converted); assertEquals( - TypeCreator.NULLABLE.I32, - assertInstanceOf(Expression.NullLiteral.class, converted).getType()); + Integer.MAX_VALUE, ((FieldReference.ListElement) reference.segments().get(0)).offset()); + assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); + verifyRoundTrip(converted, TypeConverter.DEFAULT.toSubstrait(input.getType())); } } @@ -182,6 +194,84 @@ private RexNode nullableIndex(Integer index) { return index == null ? rexBuilder.makeNullLiteral(intType) : integer(index); } + @Test + void invalidIndexesRemainReferencesInsideCollections() { + RexNode column = rexBuilder.makeInputRef(array().getType(), 0); + RexNode missing = rexBuilder.makeCall(SqlStdOperatorTable.ITEM, column, integer(0)); + Expression list = + rexBuilder + .makeCall(SqlStdOperatorTable.ARRAY_VALUE_CONSTRUCTOR, integer(1), missing) + .accept(converter); + assertInstanceOf(Expression.NestedList.class, list); + verifyRoundTrip(list, TypeConverter.DEFAULT.toSubstrait(column.getType())); + Expression map = + rexBuilder + .makeCall( + SqlStdOperatorTable.MAP_VALUE_CONSTRUCTOR, missing, rexBuilder.makeLiteral("x")) + .accept(converter); + assertInstanceOf(Expression.NestedMap.class, map); + verifyRoundTrip(map, TypeConverter.DEFAULT.toSubstrait(column.getType())); + + RelDataType nestedType = + typeFactory.createArrayType(typeFactory.createArrayType(array().getType(), -1), -1); + RexNode nested = rexBuilder.makeInputRef(nestedType, 0); + for (int index : new int[] {0, 1, 0}) { + nested = rexBuilder.makeCall(SqlStdOperatorTable.ITEM, nested, integer(index)); + } + FieldReference reference = assertInstanceOf(FieldReference.class, nested.accept(converter)); + assertEquals(4, reference.segments().size()); + assertEquals(TypeCreator.NULLABLE.I32, reference.getType()); + verifyRoundTrip(reference, TypeConverter.DEFAULT.toSubstrait(nestedType)); + } + + @Test + void rowElementTypesRemainIdenticalForValidAndInvalidIndexes() { + RelDataType row = typeFactory.builder().add("x", intType).build(); + RexNode column = rexBuilder.makeInputRef(typeFactory.createArrayType(row, -1), 0); + RexNode first = rexBuilder.makeCall(SqlStdOperatorTable.ITEM, column, integer(1)); + RexNode missing = rexBuilder.makeCall(SqlStdOperatorTable.ITEM, column, integer(0)); + Expression list = + rexBuilder + .makeCall(SqlStdOperatorTable.ARRAY_VALUE_CONSTRUCTOR, missing, first) + .accept(converter); + assertEquals(first.accept(converter).getType(), missing.accept(converter).getType()); + assertInstanceOf(Expression.NestedList.class, list); + verifyRoundTrip(list, TypeConverter.DEFAULT.toSubstrait(column.getType())); + } + + @Test + void requiredUserDefinedElementsCanBeAccessedWithInvalidIndexes() { + UserTypeFactory factory = new UserTypeFactory("extension:test:array", "element"); + TypeConverter types = + new TypeConverter( + new UserTypeMapper() { + @Override + public Type toSubstrait(RelDataType type) { + return factory.isTypeFromFactory(type) + ? factory.createSubstrait(type.isNullable(), factory.getTypeParameters(type)) + : null; + } + + @Override + public RelDataType toCalcite(Type.UserDefined type) { + return factory.createCalcite(type.nullable(), type.typeParameters()); + } + }); + RexExpressionConverter custom = + new RexExpressionConverter(null, CallConverters.defaults(types), null, types); + RexNode column = + rexBuilder.makeInputRef(typeFactory.createArrayType(factory.createCalcite(false), -1), 0); + for (Integer index : new Integer[] {0, null}) { + Expression converted = + rexBuilder + .makeCall(SqlStdOperatorTable.ITEM, column, nullableIndex(index)) + .accept(custom); + assertInstanceOf(FieldReference.class, converted); + assertEquals(factory.createSubstrait(true), converted.getType()); + verifyRoundTrip(converted, types.toSubstrait(column.getType())); + } + } + @ParameterizedTest @ValueSource(longs = {2147483649L, 4294967297L, Long.MAX_VALUE}) void unrepresentableOffsetsAreRejected(long index) { @@ -223,4 +313,18 @@ private io.substrait.proto.Expression toProto(Expression expression) { new ExpressionProtoConverter(new ExtensionCollector(), null), EmptyVisitationContext.INSTANCE); } + + private void verifyRoundTrip(Expression expression, Type inputType) { + ExtensionCollector extensions = new ExtensionCollector(); + io.substrait.proto.Expression proto = + new ExpressionProtoConverter(extensions, null).toProto(expression); + Expression restored = + new ProtoExpressionConverter( + extensions, + DefaultExtensionCatalog.DEFAULT_COLLECTION, + TypeCreator.REQUIRED.struct(inputType), + null) + .from(proto); + assertEquals(expression, restored); + } } From 0900c15ec82f4e3c007609d7717725e94799110f Mon Sep 17 00:00:00 2001 From: bvolpato Date: Wed, 7 Oct 2026 02:04:38 -0400 Subject: [PATCH 4/7] fix: preserve unbound selections and out-of-range indexes --- .../substrait/expression/FieldReference.java | 4 ++-- .../FieldReferenceResolveTypeTest.java | 8 ++++++++ .../relation/RelCopyOnWriteVisitorTest.java | 11 ++++++++++ .../expression/FieldSelectionConverter.java | 12 +++++------ .../FieldSelectionConverterTest.java | 20 +++++++++++++++++-- 5 files changed, 44 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/io/substrait/expression/FieldReference.java b/core/src/main/java/io/substrait/expression/FieldReference.java index 55a8b47aa..257508e66 100644 --- a/core/src/main/java/io/substrait/expression/FieldReference.java +++ b/core/src/main/java/io/substrait/expression/FieldReference.java @@ -603,7 +603,7 @@ public Type visit(Type.Struct expr) throws RuntimeException { throw new IllegalArgumentException("Undefined struct type."); } Type field = expr.fields().get(index); - return field.withNullable(field.nullable() || expr.nullable()); + return expr.nullable() ? field.withNullable(true) : field; } public static Type getReferencedType(Type type, int index) { @@ -648,7 +648,7 @@ public Type visit(Type.Map expr) throws RuntimeException { "Key type %s of map does not matched expected type of %s.", expr.key(), keyType)); } Type value = expr.value(); - return value.withNullable(value.nullable() || expr.nullable()); + return expr.nullable() ? value.withNullable(true) : value; } public static Type getReferencedType(Type typeToDereference, Type keyType) { diff --git a/core/src/test/java/io/substrait/expression/FieldReferenceResolveTypeTest.java b/core/src/test/java/io/substrait/expression/FieldReferenceResolveTypeTest.java index 50266bf40..e24a6f4ce 100644 --- a/core/src/test/java/io/substrait/expression/FieldReferenceResolveTypeTest.java +++ b/core/src/test/java/io/substrait/expression/FieldReferenceResolveTypeTest.java @@ -46,6 +46,10 @@ static Stream cases() { return Stream.of( // struct field, single segment Arguments.of("struct field in range", R.struct(R.I64), segments(StructField.of(0))), + Arguments.of( + "unbound struct field", + R.struct(Type.Unbound.builder().build()), + segments(StructField.of(0))), Arguments.of( "struct field, second column", R.struct(R.I64, R.STRING), segments(StructField.of(1))), Arguments.of("struct field past the end", R.struct(R.I64), segments(StructField.of(2))), @@ -118,6 +122,10 @@ static Stream cases() { "nullable map parent", R.struct(N.map(R.STRING, R.I64)), segments(key("k"), StructField.of(0))), + Arguments.of( + "unbound map value", + R.struct(R.map(R.STRING, Type.Unbound.builder().build())), + segments(key("k"), StructField.of(0))), Arguments.of( "map key differing in nullability", R.struct(R.map(N.STRING, R.I64)), diff --git a/core/src/test/java/io/substrait/relation/RelCopyOnWriteVisitorTest.java b/core/src/test/java/io/substrait/relation/RelCopyOnWriteVisitorTest.java index 8ecb45063..691ec9879 100644 --- a/core/src/test/java/io/substrait/relation/RelCopyOnWriteVisitorTest.java +++ b/core/src/test/java/io/substrait/relation/RelCopyOnWriteVisitorTest.java @@ -41,6 +41,17 @@ */ class RelCopyOnWriteVisitorTest extends TestBase { + @Test + void identityVisitorPreservesUnboundColumn() { + Rel input = scan("t", Type.Unbound.builder().build()); + Rel plan = sb.project(in -> Arrays.asList(sb.fieldReference(in, 0)), input); + + assertEquals( + Optional.empty(), + plan.accept( + new RelCopyOnWriteVisitor() {}, EmptyVisitationContext.INSTANCE)); + } + private Rel scan(String table, Type... columnTypes) { return sb.namedScan( Arrays.asList(table), diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java index 048f18c91..9ff8673ef 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java @@ -73,13 +73,12 @@ public Optional convert( return Optional.empty(); } - Literal literal = (new LiteralConverter(typeConverter)).convert((RexLiteral) reference); - Expression input = topLevelConverter.apply(toDereference); switch (toDereference.getType().getSqlTypeName()) { case ROW: { + Literal literal = (new LiteralConverter(typeConverter)).convert((RexLiteral) reference); Optional index = toInt(literal); if (index.isEmpty()) { return Optional.empty(); @@ -102,7 +101,8 @@ public Optional convert( return Optional.empty(); } long offset = Integer.MAX_VALUE; - if (!(literal instanceof Expression.NullLiteral)) { + if (!((RexLiteral) reference).isNull()) { + Literal literal = (new LiteralConverter(typeConverter)).convert((RexLiteral) reference); Optional index = toLong(literal); if (index.isEmpty()) { return Optional.empty(); @@ -112,12 +112,9 @@ public Optional convert( // zero-based offset cannot select an element. Keep the reference to evaluate the // array operand and preserve its element type, including for a null index. if (index.get() >= operator.offset) { - offset = index.get() - operator.offset; + offset = Math.min(index.get() - operator.offset, Integer.MAX_VALUE); } } - if (offset > Integer.MAX_VALUE) { - return Optional.empty(); - } if (input instanceof FieldReference) { return Optional.of(((FieldReference) input).dereferenceList((int) offset)); @@ -128,6 +125,7 @@ public Optional convert( case MAP: { + Literal literal = (new LiteralConverter(typeConverter)).convert((RexLiteral) reference); Optional mapKey = toString(literal); if (mapKey.isEmpty()) { return Optional.empty(); diff --git a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java index ab52d4ad6..77d8c89b4 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java @@ -122,6 +122,20 @@ void nullIndexIsNull() { assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); } + @Test + void untypedNullIndexIsNull() { + Expression converted = + rexBuilder + .makeCall( + SqlStdOperatorTable.ITEM, + array(), + rexBuilder.makeNullLiteral(typeFactory.createSqlType(SqlTypeName.NULL))) + .accept(converter); + assertEquals(Integer.MAX_VALUE, listOffset(converted)); + assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); + verifyRoundTrip(converted, TypeConverter.DEFAULT.toSubstrait(array().getType())); + } + @ParameterizedTest @MethodSource("nullOrInvalidIndexes") void retainsThrowingArray(SqlOperator operator, Integer index) { @@ -274,9 +288,11 @@ public RelDataType toCalcite(Type.UserDefined type) { @ParameterizedTest @ValueSource(longs = {2147483649L, 4294967297L, Long.MAX_VALUE}) - void unrepresentableOffsetsAreRejected(long index) { + void offsetsPastTheListLengthLimitAreNull(long index) { RexNode call = rexBuilder.makeCall(SqlStdOperatorTable.ITEM, array(), integer(index)); - assertThrows(IllegalArgumentException.class, () -> call.accept(converter)); + Expression converted = call.accept(converter); + assertEquals(Integer.MAX_VALUE, listOffset(converted)); + assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); } @ParameterizedTest From 5affd6d91cd12e359f3bf0843de7646e394764a6 Mon Sep 17 00:00:00 2001 From: bvolpato Date: Thu, 8 Oct 2026 20:34:13 +0000 Subject: [PATCH 5/7] revert(core): remove the selection nullability change The change to the derived type of list, struct and map selections needs its own review. The array index translation in isthmus does not depend on it. --- .../substrait/expression/FieldReference.java | 26 +++++------- .../FieldReferenceResolveTypeTest.java | 28 +------------ .../relation/RelCopyOnWriteVisitorTest.java | 13 +----- .../proto/FieldReferenceRoundtripTest.java | 40 ------------------- 4 files changed, 14 insertions(+), 93 deletions(-) diff --git a/core/src/main/java/io/substrait/expression/FieldReference.java b/core/src/main/java/io/substrait/expression/FieldReference.java index 257508e66..a8ceeaff9 100644 --- a/core/src/main/java/io/substrait/expression/FieldReference.java +++ b/core/src/main/java/io/substrait/expression/FieldReference.java @@ -558,29 +558,28 @@ public static Optional resolveType(Type rootType, List s * nothing out of it. * *

This mirrors the type each segment derives when it is applied: a struct field selects the - * field at its offset, a list element selects the nullable element type whatever its offset, as - * the length of a list is not part of its type, and a map key selects the value type of a map - * whose key type it matches, nullability included. A segment applied to a type that is not the - * container it navigates into, and any other kind of segment, select nothing. + * field at its offset, a list element selects the element type whatever its offset, as the length + * of a list is not part of its type, and a map key selects the value type of a map whose key type + * it matches, nullability included. A segment applied to a type that is not the container it + * navigates into, and any other kind of segment, select nothing. */ private static Optional resolveSegmentType(ReferenceSegment segment, Type type) { if (segment instanceof StructField && type instanceof Type.Struct) { int offset = ((StructField) segment).offset(); List fields = ((Type.Struct) type).fields(); return offset >= 0 && offset < fields.size() - ? Optional.of(StructFieldFinder.getReferencedType(type, offset)) + ? Optional.of(fields.get(offset)) : Optional.empty(); } if (segment instanceof ListElement && type instanceof Type.ListType) { - return Optional.of(ListIndexFinder.getReferencedType(type, ((ListElement) segment).offset())); + return Optional.of(((Type.ListType) type).elementType()); } if (segment instanceof MapKey && type instanceof Type.Map) { // The type of the key literal is only read once the type is known to be a map, which is also // the only case in which applying the segment reads it. Type.Map map = (Type.Map) type; - Type keyType = ((MapKey) segment).key().getType(); - return map.key().equals(keyType) - ? Optional.of(MapKeyFinder.getReferencedType(type, keyType)) + return map.key().equals(((MapKey) segment).key().getType()) + ? Optional.of(map.value()) : Optional.empty(); } return Optional.empty(); @@ -602,8 +601,7 @@ public Type visit(Type.Struct expr) throws RuntimeException { if (expr.fields().size() < index) { throw new IllegalArgumentException("Undefined struct type."); } - Type field = expr.fields().get(index); - return expr.nullable() ? field.withNullable(true) : field; + return expr.fields().get(index); } public static Type getReferencedType(Type type, int index) { @@ -620,8 +618,7 @@ private ListIndexFinder() { @Override public Type visit(Type.ListType expr) throws RuntimeException { - // An out-of-range offset returns null even when the list's elements are required. - return expr.elementType().withNullable(true); + return expr.elementType(); } public static Type getReferencedType(Type type, int index) { @@ -647,8 +644,7 @@ public Type visit(Type.Map expr) throws RuntimeException { String.format( "Key type %s of map does not matched expected type of %s.", expr.key(), keyType)); } - Type value = expr.value(); - return expr.nullable() ? value.withNullable(true) : value; + return expr.value(); } public static Type getReferencedType(Type typeToDereference, Type keyType) { diff --git a/core/src/test/java/io/substrait/expression/FieldReferenceResolveTypeTest.java b/core/src/test/java/io/substrait/expression/FieldReferenceResolveTypeTest.java index e24a6f4ce..d45c73b3c 100644 --- a/core/src/test/java/io/substrait/expression/FieldReferenceResolveTypeTest.java +++ b/core/src/test/java/io/substrait/expression/FieldReferenceResolveTypeTest.java @@ -29,7 +29,7 @@ * throw in that case. The two must agree on which chains select something, so that a * caller re-deriving a cached type can tell a reference that no longer resolves from a failure of * its own work. Asserting the equivalence rather than hard-coding expectations is what keeps {@code - * resolveType} from drifting away from the shared segment derivation rules. + * resolveType} from drifting away from the segment derivation rules it duplicates. */ class FieldReferenceResolveTypeTest extends TestBase { @@ -46,10 +46,6 @@ static Stream cases() { return Stream.of( // struct field, single segment Arguments.of("struct field in range", R.struct(R.I64), segments(StructField.of(0))), - Arguments.of( - "unbound struct field", - R.struct(Type.Unbound.builder().build()), - segments(StructField.of(0))), Arguments.of( "struct field, second column", R.struct(R.I64, R.STRING), segments(StructField.of(1))), Arguments.of("struct field past the end", R.struct(R.I64), segments(StructField.of(2))), @@ -61,10 +57,6 @@ static Stream cases() { "nested struct field in range", R.struct(R.struct(R.I64, R.STRING)), segments(StructField.of(1), StructField.of(0))), - Arguments.of( - "nullable struct parent", - R.struct(N.struct(R.I64)), - segments(StructField.of(0), StructField.of(0))), Arguments.of( "nested struct field past the end", R.struct(R.struct(R.I64)), @@ -99,14 +91,6 @@ static Stream cases() { "list element offset is not bounds checked", R.struct(R.list(R.I64)), segments(ListElement.of(7), StructField.of(0))), - Arguments.of( - "struct field beneath a list element", - R.struct(R.list(R.struct(R.I64))), - segments(StructField.of(0), ListElement.of(0), StructField.of(0))), - Arguments.of( - "map value beneath a list element", - R.struct(R.list(R.map(R.STRING, R.I64))), - segments(key("k"), ListElement.of(0), StructField.of(0))), Arguments.of( "list element on a struct", R.struct(R.struct(R.I64)), @@ -118,14 +102,6 @@ static Stream cases() { "map key matching the key type", R.struct(R.map(R.STRING, R.I64)), segments(key("k"), StructField.of(0))), - Arguments.of( - "nullable map parent", - R.struct(N.map(R.STRING, R.I64)), - segments(key("k"), StructField.of(0))), - Arguments.of( - "unbound map value", - R.struct(R.map(R.STRING, Type.Unbound.builder().build())), - segments(key("k"), StructField.of(0))), Arguments.of( "map key differing in nullability", R.struct(R.map(N.STRING, R.I64)), @@ -185,7 +161,7 @@ void resolvesAgainstRootsThatAreNotStructs() { // ofRoot only accepts a struct, but a reference rooted at an expression can navigate into a // list or a map directly, so resolveType has to accept any type as the root. assertEquals( - Optional.of(N.I64), FieldReference.resolveType(R.list(R.I64), segments(ListElement.of(0)))); + Optional.of(R.I64), FieldReference.resolveType(R.list(R.I64), segments(ListElement.of(0)))); assertEquals( Optional.of(R.I64), FieldReference.resolveType(R.map(R.STRING, R.I64), segments(key("k")))); assertFalse(FieldReference.resolveType(R.I64, segments(StructField.of(0))).isPresent()); diff --git a/core/src/test/java/io/substrait/relation/RelCopyOnWriteVisitorTest.java b/core/src/test/java/io/substrait/relation/RelCopyOnWriteVisitorTest.java index 691ec9879..a4e001e3c 100644 --- a/core/src/test/java/io/substrait/relation/RelCopyOnWriteVisitorTest.java +++ b/core/src/test/java/io/substrait/relation/RelCopyOnWriteVisitorTest.java @@ -41,17 +41,6 @@ */ class RelCopyOnWriteVisitorTest extends TestBase { - @Test - void identityVisitorPreservesUnboundColumn() { - Rel input = scan("t", Type.Unbound.builder().build()); - Rel plan = sb.project(in -> Arrays.asList(sb.fieldReference(in, 0)), input); - - assertEquals( - Optional.empty(), - plan.accept( - new RelCopyOnWriteVisitor() {}, EmptyVisitationContext.INSTANCE)); - } - private Rel scan(String table, Type... columnTypes) { return sb.namedScan( Arrays.asList(table), @@ -513,7 +502,7 @@ void listElementSegmentIsRetyped() { @Test void listElementSegmentOnANonListIsLeftAlone() { assertEquals( - N.STRING, + R.STRING, rewriteNestedReference(R.list(R.STRING), R.struct(R.I64), ListElement.of(0)).getType()); } diff --git a/core/src/test/java/io/substrait/type/proto/FieldReferenceRoundtripTest.java b/core/src/test/java/io/substrait/type/proto/FieldReferenceRoundtripTest.java index 71243a396..4e00797c2 100644 --- a/core/src/test/java/io/substrait/type/proto/FieldReferenceRoundtripTest.java +++ b/core/src/test/java/io/substrait/type/proto/FieldReferenceRoundtripTest.java @@ -1,18 +1,13 @@ package io.substrait.type.proto; -import static org.junit.jupiter.api.Assertions.assertEquals; - import io.substrait.TestBase; import io.substrait.expression.Expression; -import io.substrait.expression.ExpressionCreator; -import io.substrait.expression.FieldReference; import io.substrait.relation.Filter; import io.substrait.relation.Project; import io.substrait.relation.Rel; import io.substrait.type.Type; import java.util.Arrays; import java.util.Collections; -import java.util.List; import org.junit.jupiter.api.Test; /** @@ -163,39 +158,4 @@ void sameFieldReferencedMultipleTimes() { verifyRoundTrip(projection); } - - @Test - void listSelectionsRemainNullableThroughNestedFields() { - Rel input = - sb.namedScan( - List.of("arrays"), - List.of("lists", "rows", "maps"), - List.of( - R.list(R.list(R.I32)), R.list(R.struct(R.I32)), R.list(R.map(R.STRING, R.I32)))); - for (int offset : new int[] {0, -1, Integer.MAX_VALUE}) { - List selections = - List.of( - FieldReference.newInputRelReference(0, input) - .dereferenceList(offset) - .dereferenceList(0), - FieldReference.newInputRelReference(1, input) - .dereferenceList(offset) - .dereferenceStruct(0), - FieldReference.newInputRelReference(2, input) - .dereferenceList(offset) - .dereferenceMap(ExpressionCreator.string(false, "key"))); - selections.forEach(selection -> assertEquals(N.I32, selection.getType())); - verifyRoundTrip(Project.builder().input(input).expressions(selections).build()); - } - } - - @Test - void listSelectionOnExpressionRemainsNullable() { - Expression input = ExpressionCreator.list(false, ExpressionCreator.i32(false, 1)); - for (int offset : new int[] {0, Integer.MAX_VALUE}) { - FieldReference selection = FieldReference.newListReference(offset, input); - assertEquals(N.I32, selection.getType()); - verifyRoundTrip(selection); - } - } } From 09da9d6001d563a5b9544fa1d8f06838db451b0c Mon Sep 17 00:00:00 2001 From: bvolpato Date: Thu, 8 Oct 2026 20:34:15 +0000 Subject: [PATCH 6/7] fix(isthmus): decline unsafe array operators before operand conversion Check the array operator before the array operand is converted, so a declined call does not convert the operand first. Assert that the converted segment is a list element before the tests read its offset. Protobuf returns offset 0 for a missing segment. Remove the type assertions that depended on the core nullability change. --- .../expression/FieldSelectionConverter.java | 27 ++++++++---- .../FieldSelectionConverterTest.java | 41 +++++++++++-------- 2 files changed, 43 insertions(+), 25 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java index 9ff8673ef..de6719440 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java @@ -12,7 +12,9 @@ import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.fun.SqlItemOperator; +import org.apache.calcite.sql.type.SqlTypeName; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -73,9 +75,16 @@ public Optional convert( return Optional.empty(); } + SqlTypeName containerType = toDereference.getType().getSqlTypeName(); + // A Substrait list reference returns null for an out-of-range index, so it cannot preserve + // the error behavior of OFFSET or ORDINAL. Decline before the array operand is converted. + if (containerType == SqlTypeName.ARRAY && !isSafeArrayOperator(call.getOperator())) { + return Optional.empty(); + } + Expression input = topLevelConverter.apply(toDereference); - switch (toDereference.getType().getSqlTypeName()) { + switch (containerType) { case ROW: { Literal literal = (new LiteralConverter(typeConverter)).convert((RexLiteral) reference); @@ -91,15 +100,7 @@ public Optional convert( } case ARRAY: { - if (!(call.getOperator() instanceof SqlItemOperator)) { - return Optional.empty(); - } SqlItemOperator operator = (SqlItemOperator) call.getOperator(); - // A Substrait list reference returns null for an out-of-range index, so it cannot - // preserve the error behavior of OFFSET or ORDINAL. - if (!operator.safe || (operator.offset != 0 && operator.offset != 1)) { - return Optional.empty(); - } long offset = Integer.MAX_VALUE; if (!((RexLiteral) reference).isNull()) { Literal literal = (new LiteralConverter(typeConverter)).convert((RexLiteral) reference); @@ -143,6 +144,14 @@ public Optional convert( return Optional.empty(); } + private static boolean isSafeArrayOperator(SqlOperator operator) { + if (!(operator instanceof SqlItemOperator)) { + return false; + } + SqlItemOperator itemOperator = (SqlItemOperator) operator; + return itemOperator.safe && (itemOperator.offset == 0 || itemOperator.offset == 1); + } + /** * Converts a numeric literal to an integer index. * diff --git a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java index 77d8c89b4..6c8f75f5d 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java @@ -3,6 +3,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import io.substrait.expression.Expression; import io.substrait.expression.FieldReference; @@ -18,11 +19,13 @@ import io.substrait.util.EmptyVisitationContext; import java.math.BigDecimal; import java.util.List; +import java.util.Optional; import java.util.stream.Stream; import org.apache.calcite.DataContexts; import org.apache.calcite.jdbc.JavaTypeFactoryImpl; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexCall; import org.apache.calcite.rex.RexExecutable; import org.apache.calcite.rex.RexExecutorImpl; import org.apache.calcite.rex.RexNode; @@ -55,7 +58,6 @@ void itemPreservesCalciteIndexing(int index, Integer expectedOffset, Integer exp Expression converted = call.accept(converter); assertEquals( expectedOffset == null ? Integer.MAX_VALUE : expectedOffset, listOffset(converted)); - assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); } @ParameterizedTest @@ -72,13 +74,7 @@ void safeOperatorsUseTheirOwnBase(SqlOperator operator, long index, int expected io.substrait.proto.Expression proto = toProto(columnExpression); assertEquals( expectedOffset, - proto - .getSelection() - .getDirectReference() - .getStructField() - .getChild() - .getListElement() - .getOffset()); + listOffset(proto.getSelection().getDirectReference().getStructField().getChild())); } private static Stream safeIndexing() { @@ -99,7 +95,6 @@ private static Stream safeIndexing() { void invalidLowIndexesAreNull(SqlOperator operator, long index) { Expression converted = rexBuilder.makeCall(operator, array(), integer(index)).accept(converter); assertEquals(Integer.MAX_VALUE, listOffset(converted)); - assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); } private static Stream invalidLowIndexes() { @@ -119,7 +114,6 @@ void nullIndexIsNull() { .makeCall(SqlStdOperatorTable.ITEM, array(), rexBuilder.makeNullLiteral(intType)) .accept(converter); assertEquals(Integer.MAX_VALUE, listOffset(converted)); - assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); } @Test @@ -132,7 +126,6 @@ void untypedNullIndexIsNull() { rexBuilder.makeNullLiteral(typeFactory.createSqlType(SqlTypeName.NULL))) .accept(converter); assertEquals(Integer.MAX_VALUE, listOffset(converted)); - assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); verifyRoundTrip(converted, TypeConverter.DEFAULT.toSubstrait(array().getType())); } @@ -188,7 +181,6 @@ void literalAndColumnArraysRetainReferences(SqlOperator operator, Integer index) FieldReference reference = assertInstanceOf(FieldReference.class, converted); assertEquals( Integer.MAX_VALUE, ((FieldReference.ListElement) reference.segments().get(0)).offset()); - assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); verifyRoundTrip(converted, TypeConverter.DEFAULT.toSubstrait(input.getType())); } } @@ -234,7 +226,6 @@ void invalidIndexesRemainReferencesInsideCollections() { } FieldReference reference = assertInstanceOf(FieldReference.class, nested.accept(converter)); assertEquals(4, reference.segments().size()); - assertEquals(TypeCreator.NULLABLE.I32, reference.getType()); verifyRoundTrip(reference, TypeConverter.DEFAULT.toSubstrait(nestedType)); } @@ -281,7 +272,6 @@ public RelDataType toCalcite(Type.UserDefined type) { .makeCall(SqlStdOperatorTable.ITEM, column, nullableIndex(index)) .accept(custom); assertInstanceOf(FieldReference.class, converted); - assertEquals(factory.createSubstrait(true), converted.getType()); verifyRoundTrip(converted, types.toSubstrait(column.getType())); } } @@ -292,7 +282,6 @@ void offsetsPastTheListLengthLimitAreNull(long index) { RexNode call = rexBuilder.makeCall(SqlStdOperatorTable.ITEM, array(), integer(index)); Expression converted = call.accept(converter); assertEquals(Integer.MAX_VALUE, listOffset(converted)); - assertEquals(TypeCreator.NULLABLE.I32, converted.getType()); } @ParameterizedTest @@ -302,6 +291,20 @@ void throwingOperatorsAreRejected(SqlOperator operator) { assertThrows(IllegalArgumentException.class, () -> call.accept(converter)); } + @ParameterizedTest + @MethodSource("unsafeOperators") + void declinedOperatorsDoNotConvertTheArrayOperand(SqlOperator operator) { + RexNode call = rexBuilder.makeCall(operator, array(), integer(1)); + assertEquals( + Optional.empty(), + new FieldSelectionConverter(TypeConverter.DEFAULT) + .convert( + (RexCall) call, + operand -> { + throw new AssertionError("converted " + operand); + })); + } + private static Stream unsafeOperators() { return Stream.of(SqlLibraryOperators.OFFSET, SqlLibraryOperators.ORDINAL); } @@ -321,7 +324,13 @@ private RexNode integer(long value) { private int listOffset(Expression expression) { assertInstanceOf(FieldReference.class, expression); - return toProto(expression).getSelection().getDirectReference().getListElement().getOffset(); + return listOffset(toProto(expression).getSelection().getDirectReference()); + } + + private int listOffset(io.substrait.proto.Expression.ReferenceSegment segment) { + // Protobuf returns offset 0 for a segment that is not a list element. + assertTrue(segment.hasListElement()); + return segment.getListElement().getOffset(); } private io.substrait.proto.Expression toProto(Expression expression) { From 0af315104b494a0ea96ee298631cf74e48903127 Mon Sep 17 00:00:00 2001 From: bvolpato Date: Sat, 10 Oct 2026 18:23:32 -0400 Subject: [PATCH 7/7] test(isthmus): pin nested array selection offsets --- .../isthmus/expression/FieldSelectionConverter.java | 2 +- .../isthmus/expression/FieldSelectionConverterTest.java | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java index de6719440..b18a53dba 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java @@ -46,7 +46,7 @@ public FieldSelectionConverter(TypeConverter typeConverter) { * *

    *
  • ROW dereference by integer index - *
  • ARRAY dereference by integer index, preserving the safe operator's indexing base + *
  • ARRAY dereference by integer index, translated to Substrait's zero-based offset *
  • MAP dereference by string key *
* diff --git a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java index 6c8f75f5d..7f81f0359 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java @@ -226,6 +226,15 @@ void invalidIndexesRemainReferencesInsideCollections() { } FieldReference reference = assertInstanceOf(FieldReference.class, nested.accept(converter)); assertEquals(4, reference.segments().size()); + assertEquals( + Integer.MAX_VALUE, + assertInstanceOf(FieldReference.ListElement.class, reference.segments().get(0)).offset()); + assertEquals( + 0, + assertInstanceOf(FieldReference.ListElement.class, reference.segments().get(1)).offset()); + assertEquals( + Integer.MAX_VALUE, + assertInstanceOf(FieldReference.ListElement.class, reference.segments().get(2)).offset()); verifyRoundTrip(reference, TypeConverter.DEFAULT.toSubstrait(nestedType)); }