From f4b71476e9455d1c6b66b760a294984a6a8da370 Mon Sep 17 00:00:00 2001 From: Ben Bellick Date: Thu, 6 Aug 2026 17:08:18 -0400 Subject: [PATCH 1/3] feat: enforce offset/count exprs in FetchRel are ints --- .../java/io/substrait/relation/Fetch.java | 26 ++++++++ .../src/main/java/io/substrait/type/Type.java | 12 ++++ .../java/io/substrait/relation/FetchTest.java | 64 +++++++++++++++++++ .../test/java/io/substrait/type/TypeTest.java | 61 ++++++++++++++++++ 4 files changed, 163 insertions(+) create mode 100644 core/src/test/java/io/substrait/relation/FetchTest.java create mode 100644 core/src/test/java/io/substrait/type/TypeTest.java diff --git a/core/src/main/java/io/substrait/relation/Fetch.java b/core/src/main/java/io/substrait/relation/Fetch.java index 6e510c7ea..694670289 100644 --- a/core/src/main/java/io/substrait/relation/Fetch.java +++ b/core/src/main/java/io/substrait/relation/Fetch.java @@ -29,6 +29,32 @@ public abstract class Fetch extends SingleInputRel implements HasExtension { */ public abstract Optional getCount(); + @Value.Check + protected void check() { + getOffset() + .ifPresent( + offset -> { + Type type = offset.getType(); + if (!type.isInteger()) { + throw new IllegalArgumentException( + "Fetch offset expression must have an integer type " + + "(I8, I16, I32 or I64), but got: " + + type); + } + }); + getCount() + .ifPresent( + count -> { + Type type = count.getType(); + if (!type.isInteger()) { + throw new IllegalArgumentException( + "Fetch count expression must have an integer type " + + "(I8, I16, I32 or I64), but got: " + + type); + } + }); + } + @Override protected Type.Struct deriveRecordType() { return getInput().getRecordType(); diff --git a/core/src/main/java/io/substrait/type/Type.java b/core/src/main/java/io/substrait/type/Type.java index abfb80b0c..763127207 100644 --- a/core/src/main/java/io/substrait/type/Type.java +++ b/core/src/main/java/io/substrait/type/Type.java @@ -53,6 +53,18 @@ default boolean equalsIgnoringNullability(Type other) { return TypeCreator.asNullable(this).equals(TypeCreator.asNullable(other)); } + /** + * Returns whether this is one of the integer types {@link I8}, {@link I16}, {@link I32} or {@link + * I64}. The Substrait spec only "recommends" {@code i64} for contexts such as {@code FetchRel}'s + * {@code offset_expr}/{@code count_expr}, so callers that accept any integer width can use this + * to reject non-integer types. + * + * @return {@code true} if this is an integer type + */ + default boolean isInteger() { + return this instanceof I8 || this instanceof I16 || this instanceof I32 || this instanceof I64; + } + /** The boolean type. */ @Value.Immutable abstract class Bool implements Type { diff --git a/core/src/test/java/io/substrait/relation/FetchTest.java b/core/src/test/java/io/substrait/relation/FetchTest.java new file mode 100644 index 000000000..1302fb5eb --- /dev/null +++ b/core/src/test/java/io/substrait/relation/FetchTest.java @@ -0,0 +1,64 @@ +package io.substrait.relation; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import io.substrait.TestBase; +import java.util.Arrays; +import org.junit.jupiter.api.Test; + +/** + * Validation tests for {@link Fetch}, whose offset/count expressions must have an integer type. + * Round-trip coverage lives in the {@code io.substrait.type.proto} package. + */ +class FetchTest extends TestBase { + + // Reuse the same schema shape as FetchRoundtripTest so the two files stay in sync. + final Rel table = + sb.namedScan(Arrays.asList("T"), Arrays.asList("a", "b"), Arrays.asList(R.I64, R.STRING)); + + /** Every integer width is an acceptable offset/count expression type. */ + @Test + void integerWidthsAccepted() { + fetch().offset(sb.i8(1)).count(sb.i8(2)).build(); + fetch().offset(sb.i16(1)).count(sb.i16(2)).build(); + fetch().offset(sb.i32(1)).count(sb.i32(2)).build(); + fetch().offset(sb.i64(1)).count(sb.i64(2)).build(); + } + + /** A non-integer offset expression is rejected at construction time. */ + @Test + void nonIntegerOffsetRejected() { + assertThrows(IllegalArgumentException.class, () -> fetch().offset(sb.fp64(1.0)).build()); + } + + /** A non-integer count expression is rejected at construction time. */ + @Test + void nonIntegerCountRejected() { + assertThrows(IllegalArgumentException.class, () -> fetch().count(sb.fp64(1.0)).build()); + } + + /** A non-integer type reaches the check via the proto conversion path too. */ + @Test + void nonIntegerOffsetRejectedViaProto() { + // Build a FetchRel proto directly with a non-integer offset_expr so the Fetch POJO check is + // exercised by ProtoRelConverter rather than by direct construction. + io.substrait.proto.Rel inputProto = relProtoConverter.toProto(table); + io.substrait.proto.Expression fp64Expr = + io.substrait.proto.Expression.newBuilder() + .setLiteral(io.substrait.proto.Expression.Literal.newBuilder().setFp64(1.0).build()) + .build(); + io.substrait.proto.FetchRel fetchRel = + io.substrait.proto.FetchRel.newBuilder() + .setCommon(io.substrait.proto.RelCommon.newBuilder().build()) + .setInput(inputProto) + .setOffsetExpr(fp64Expr) + .build(); + io.substrait.proto.Rel protoRel = + io.substrait.proto.Rel.newBuilder().setFetch(fetchRel).build(); + assertThrows(IllegalArgumentException.class, () -> protoRelConverter.from(protoRel)); + } + + private ImmutableFetch.Builder fetch() { + return Fetch.builder().input(table); + } +} diff --git a/core/src/test/java/io/substrait/type/TypeTest.java b/core/src/test/java/io/substrait/type/TypeTest.java new file mode 100644 index 000000000..c331659eb --- /dev/null +++ b/core/src/test/java/io/substrait/type/TypeTest.java @@ -0,0 +1,61 @@ +package io.substrait.type; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +/** Tests for the default methods on {@link Type}. */ +class TypeTest { + private static final TypeCreator R = TypeCreator.REQUIRED; + private static final TypeCreator N = TypeCreator.NULLABLE; + + @Test + void integerWidthsAreIntegers() { + assertTrue(R.I8.isInteger()); + assertTrue(R.I16.isInteger()); + assertTrue(R.I32.isInteger()); + assertTrue(R.I64.isInteger()); + } + + @Test + void nullabilityIsIrrelevantForIsInteger() { + assertTrue(N.I8.isInteger()); + assertTrue(N.I16.isInteger()); + assertTrue(N.I32.isInteger()); + assertTrue(N.I64.isInteger()); + } + + @Test + void nonIntegerPrimitivesAreNotIntegers() { + assertFalse(R.BOOLEAN.isInteger()); + assertFalse(R.FP32.isInteger()); + assertFalse(R.FP64.isInteger()); + assertFalse(R.STRING.isInteger()); + assertFalse(R.BINARY.isInteger()); + assertFalse(R.DATE.isInteger()); + assertFalse(R.UUID.isInteger()); + } + + @Test + void compoundAndSpecialTypesAreNotIntegers() { + assertFalse(R.decimal(10, 2).isInteger()); + assertFalse(R.struct(R.I64).isInteger()); + assertFalse(R.list(R.I64).isInteger()); + assertFalse(R.map(R.I64, R.I64).isInteger()); + } + + @Test + void equalsIgnoringNullabilityMatchesAcrossNullability() { + assertTrue(R.I64.equalsIgnoringNullability(N.I64)); + assertTrue(N.I64.equalsIgnoringNullability(R.I64)); + assertTrue(R.I64.equalsIgnoringNullability(R.I64)); + } + + @Test + void equalsIgnoringNullabilityDistinguishesKinds() { + assertFalse(R.I64.equalsIgnoringNullability(R.I32)); + assertFalse(R.I64.equalsIgnoringNullability(N.FP64)); + assertFalse(R.struct(R.I64).equalsIgnoringNullability(R.I64)); + } +} From 5034e40aa17031c86ee8f4b3dadade791e40d50d Mon Sep 17 00:00:00 2001 From: Ben Bellick Date: Fri, 7 Aug 2026 17:15:30 -0400 Subject: [PATCH 2/3] refactor(core): collapse Fetch int-type check into a helper, render types readably Adopt review feedback: fold the duplicated offset/count branches into a requireIntegerType(Optional, field) helper that still produces per-field errors, render the offending type via StringTypeVisitor (decimal<10,2> rather than Immutables toString), and add the doclint-required Javadoc on check(). Decouple Type.isInteger()'s Javadoc from its caller so the type model no longer references FetchRel; the spec rationale lives in Fetch. --- .../java/io/substrait/relation/Fetch.java | 46 ++++++++++--------- .../src/main/java/io/substrait/type/Type.java | 6 +-- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/io/substrait/relation/Fetch.java b/core/src/main/java/io/substrait/relation/Fetch.java index 694670289..02a88cfb1 100644 --- a/core/src/main/java/io/substrait/relation/Fetch.java +++ b/core/src/main/java/io/substrait/relation/Fetch.java @@ -1,6 +1,7 @@ package io.substrait.relation; import io.substrait.expression.Expression; +import io.substrait.type.StringTypeVisitor; import io.substrait.type.Type; import io.substrait.util.VisitationContext; import java.util.Optional; @@ -29,30 +30,31 @@ public abstract class Fetch extends SingleInputRel implements HasExtension { */ public abstract Optional getCount(); + /** + * Validates that the offset and count expressions are integer-typed. Both must evaluate to a + * non-negative integer; {@code i64} is recommended but not required, so any integer width is + * accepted (spec v0.99.0). + * + * @throws IllegalArgumentException if the offset or count expression is not integer-typed + */ @Value.Check protected void check() { - getOffset() - .ifPresent( - offset -> { - Type type = offset.getType(); - if (!type.isInteger()) { - throw new IllegalArgumentException( - "Fetch offset expression must have an integer type " - + "(I8, I16, I32 or I64), but got: " - + type); - } - }); - getCount() - .ifPresent( - count -> { - Type type = count.getType(); - if (!type.isInteger()) { - throw new IllegalArgumentException( - "Fetch count expression must have an integer type " - + "(I8, I16, I32 or I64), but got: " - + type); - } - }); + requireIntegerType(getOffset(), "offset"); + requireIntegerType(getCount(), "count"); + } + + private static void requireIntegerType(Optional expression, String field) { + expression.ifPresent( + e -> { + Type type = e.getType(); + if (!type.isInteger()) { + throw new IllegalArgumentException( + "Fetch " + + field + + " expression must have an integer type (i8, i16, i32 or i64), but got: " + + type.accept(new StringTypeVisitor())); + } + }); } @Override diff --git a/core/src/main/java/io/substrait/type/Type.java b/core/src/main/java/io/substrait/type/Type.java index 763127207..f2583fb28 100644 --- a/core/src/main/java/io/substrait/type/Type.java +++ b/core/src/main/java/io/substrait/type/Type.java @@ -54,10 +54,8 @@ default boolean equalsIgnoringNullability(Type other) { } /** - * Returns whether this is one of the integer types {@link I8}, {@link I16}, {@link I32} or {@link - * I64}. The Substrait spec only "recommends" {@code i64} for contexts such as {@code FetchRel}'s - * {@code offset_expr}/{@code count_expr}, so callers that accept any integer width can use this - * to reject non-integer types. + * Returns whether this is one of the fixed-width signed integer types: {@link I8}, {@link I16}, + * {@link I32} or {@link I64}. * * @return {@code true} if this is an integer type */ From 0f7daef41900e0d79484d48c526c262b6f148614 Mon Sep 17 00:00:00 2001 From: Ben Bellick Date: Fri, 7 Aug 2026 17:35:44 -0400 Subject: [PATCH 3/3] test(core): add UserDefined isInteger case, drop unrelated equalsIgnoringNullability tests Pin UserDefined as a non-integer Type (the one non-obvious case now that Unbound is out of scope). Drop the equalsIgnoringNullability tests, which were net-new coverage for a previously untested method unrelated to this PR. --- core/src/test/java/io/substrait/type/TypeTest.java | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/core/src/test/java/io/substrait/type/TypeTest.java b/core/src/test/java/io/substrait/type/TypeTest.java index c331659eb..4dc59d88a 100644 --- a/core/src/test/java/io/substrait/type/TypeTest.java +++ b/core/src/test/java/io/substrait/type/TypeTest.java @@ -46,16 +46,7 @@ void compoundAndSpecialTypesAreNotIntegers() { } @Test - void equalsIgnoringNullabilityMatchesAcrossNullability() { - assertTrue(R.I64.equalsIgnoringNullability(N.I64)); - assertTrue(N.I64.equalsIgnoringNullability(R.I64)); - assertTrue(R.I64.equalsIgnoringNullability(R.I64)); - } - - @Test - void equalsIgnoringNullabilityDistinguishesKinds() { - assertFalse(R.I64.equalsIgnoringNullability(R.I32)); - assertFalse(R.I64.equalsIgnoringNullability(N.FP64)); - assertFalse(R.struct(R.I64).equalsIgnoringNullability(R.I64)); + void userDefinedIsNotAnInteger() { + assertFalse(R.userDefined("urn:test", "t").isInteger()); } }