feat(core): enforce Fetch offset/count expressions are integer-typed

[benbellick]

Adds enforcement that FetchRel's offset_expr/count_expr resolve to an integer type (I8/I16/I32/I64), rejecting non-integer expressions at construction. The spec only recommends i64 for these fields:

https://github.com/substrait-io/substrait/blob/v0.99.0/proto/substrait/algebra.proto#L345-L366

---

Note: This PR was developed with AI assistance. All changes have been reviewed, and I take full responsibility for this contribution.

[nielspardon]

trying to trigger CI through close / reopen. there was a long GH actions outage yesterday and the checks were not triggered

[nielspardon]

The premise checks out, and it is stronger than the description claims. Verbatim from algebra.proto at v0.99.0, the version this repo pins:

// Expression evaluated into a non-negative integer specifying the number
// of records to skip. An expression evaluating to null is treated as 0.
// Evaluating to a negative integer should result in an error.
// Recommended type for offset is int64. Unset is treated as 0.
Expression offset_expr = 5;

Integer-ness is stated as a property of the expression's result ("evaluated into a non-negative integer"); only the concrete type is softened ("Recommended type for offset is int64"). So rejecting non-integers while accepting all four widths is exactly the right reading. IllegalArgumentException over assert matches the house rule, and @Value.Check is well-trodden here.

Two things I would like to settle before merge, then smaller notes inline. I compiled every suggestion below against your head commit, so they should be safe to take as-is.

1. :core:javadoc is the CI failure

Fetch.java:33: warning: no comment -> error: warnings found and -Werror specified. core/build.gradle.kts sets -Xdoclint:all and -Xwerror on the javadoc task, so an undocumented protected method is a hard error rather than a warning. It is the local convention too: all 11 existing @Value.Check methods in :core carry Javadoc.

Worth knowing, because it changes what the red check actually tells us: the failure aborts the build before :core:compileTestJava, :core:test, :core:pmdMain and :core:spotlessCheck. The only :core tasks that ran were classes, compileJava, jar, javadoc, processResources, writeManifest — so the new tests in this PR have not executed in CI at all yet. Expect the Javadoc fix to unmask whatever is behind it. (For what it is worth, with the inline suggestions applied the full :core:test and :core:pmdMain are green locally.)

2. Should the unbound type be exempt?

This is the one I would genuinely like your read on, because it is a policy question rather than a Fetch detail.

Type.Unbound is not an integer, so construction throws. Both reachable paths, probed against this branch:

// DynamicParameter carrying an unbound type - a producer serializing `LIMIT ?` before binding
Expression p = Expression.DynamicParameter.builder()
    .type(Type.Unbound.builder().build()).parameterReference(0).build();
Fetch.builder().input(table).offset(p).build();

// FieldReference into a scan whose column type is unbound - the shape UnboundTypeRoundtripTest covers
Rel scan = sb.namedScan(List.of("U"), List.of("a"), List.of(Type.Unbound.builder().build()));
Fetch.builder().input(scan).offset(sb.fieldReference(scan, 0)).build();

Both produce:

java.lang.IllegalArgumentException: Fetch offset expression must have an integer type
    (I8, I16, I32 or I64), but got: Unbound{}
  at io.substrait.relation.Fetch.check(Fetch.java:35)
  at io.substrait.relation.ImmutableFetch.validate(ImmutableFetch.java:503)
  at io.substrait.relation.ImmutableFetch$Builder.build(ImmutableFetch.java:942)

Everything upstream accepts unbound - the scan builds, the field reference derives Unbound{}. The new check is the only thing rejecting it. And because ProtoRelConverter.from() constructs the POJO, a plan arriving over the wire with an unbound offset_expr now fails to import, not merely to build locally.

Our own contract argues for exempting it: Type.Unbound's Javadoc says a partially bound plan "may be serialized and exchanged, but it must be bound to concrete types before execution". An import-time type check is precisely what cannot be evaluated until binding.

What makes this larger than this PR: it is the first rel-level validation that inspects an expression's type, so whatever we decide sets the precedent for every future type check against partially bound plans. No @Value.Check in the repo considers Unbound today. My inline suggestion exempts it, with the one-line revert flagged if you would rather not.

3. TopN has the same fields and the same spec wording

// Expression evaluating to a non-negative integer specifying the number of
// records to skip. Null is treated as 0. Recommended type is int64.
Expression offset = 4;

Same construction, and TopN is reachable via ProtoRelConverter.newTopN, so validating one and not the other leaves the invariant half-enforced. This compiles and leaves the existing suite green (nothing in :core, :isthmus or :spark constructs a TopN that violates it):

  /**
   * 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). The {@link Type.Unbound unbound} type is also accepted, so that a
   * partially bound plan can still be serialized and exchanged.
   *
   * @throws IllegalArgumentException if the offset or count expression is neither integer-typed nor
   *     unbound
   */
  @Value.Check
  protected void check() {
    requireIntegerType(getOffset(), "offset");
    requireIntegerType(getCount(), "count");
  }

  private static void requireIntegerType(Optional<Expression> expression, String field) {
    expression.ifPresent(
        e -> {
          Type type = e.getType();
          if (!type.isInteger() && !(type instanceof Type.Unbound)) {
            throw new IllegalArgumentException(
                "TopN "
                    + field
                    + " expression must have an integer type (i8, i16, i32 or i64), but got: "
                    + type.accept(new StringTypeVisitor()));
          }
        });
  }

It also needs import io.substrait.type.StringTypeVisitor;. The duplicated helper is a little unfortunate - the two classes are in different packages so a package-private helper will not reach both, and a protected static on SingleInputRel reads worse than the duplication. Your call.

4. Round-trip coverage for the widened acceptance

The check now runs inside ProtoRelConverter.newFetch, but FetchRoundtripTest only ever uses i64. One case pins that the other widths survive proto conversion:

  @Test
  void nonI64IntegerWidthsRoundTrip() {
    verifyRoundTrip(Fetch.builder().input(table).offset(sb.i8(3)).count(sb.i32(7)).build());
  }

5. Release notes

ProtoRelConverter.from() now throws on plans it previously accepted. That is the intent, but it is a behavior break for plan consumers, and newFetch being protected for subclassing does not help - the check itself is not overridable, so there is no leniency seam. Worth deciding deliberately whether this carries a literal BREAKING CHANGE: footer; a ## Breaking change heading produces no release-notes text.

Optional follow-up

The spec sentence has a second half this PR does not cover: "Evaluating to a negative integer should result in an error." That is statically checkable for literals. If you pursue it, note that ToSubstraitRel.scala uses -1 as an internal "no limit" marker - it correctly leaves the field unset rather than emitting -1, but it is a trap worth knowing about.

[nielspardon]

Two things bundled here, since they touch the same lines.

Blocking: the missing Javadoc is the CI failure, under -Xdoclint:all -Xwerror.

For discussion: exempting unbound (point 2 in my summary). If you would rather not, drop && !(type instanceof Type.Unbound) and the second Javadoc paragraph - everything else here stands on its own.

Also collapses the duplicated branches, and renders the type through StringTypeVisitor so the message reads decimal<10,2> instead of leaking Immutables toString syntax. The current code really does produce but got: Unbound{}, per the trace in my summary.

Needs one import outside this hunk: import io.substrait.type.StringTypeVisitor;

Suggested change

	@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);
	}
	});
	}
	/**
	* 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).
	*
	* <p>The {@link Type.Unbound unbound} type is also accepted, so that a partially bound plan can
	* still be serialized and exchanged. The integer constraint can only be enforced once the type
	* has been bound.
	*
	* @throws IllegalArgumentException if the offset or count expression is neither integer-typed nor
	* unbound
	*/
	@Value.Check
	protected void check() {
	requireIntegerType(getOffset(), "offset");
	requireIntegerType(getCount(), "count");
	}
	private static void requireIntegerType(Optional<Expression> expression, String field) {
	expression.ifPresent(
	e -> {
	Type type = e.getType();
	if (!type.isInteger() && !(type instanceof Type.Unbound)) {
	throw new IllegalArgumentException(
	"Fetch "
	+ field
	+ " expression must have an integer type (i8, i16, i32 or i64), but got: "
	+ type.accept(new StringTypeVisitor()));
	}
	});
	}

[nielspardon]

The predicate itself is good. I would decouple the Javadoc from its caller, though: this is a general predicate on the public Type interface, and documenting FetchRel's recommendation here inverts the dependency - the type model should not need to know who is asking. The spec rationale belongs in Fetch.check(), where I have put it.

Suggested change

	/**
	* 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;
	}
	/**
	* 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
	*/
	default boolean isInteger() {
	return this instanceof I8 || this instanceof I16 || this instanceof I32 || this instanceof I64;
	}

[nielspardon]

Whole-file suggestion, since the pieces share imports. What changes:

• Mixed widths. integerWidthsAccepted only paired each width with itself. Worth covering because it is the case that catches a plausible refactor: a helper that compares the two types to each other rather than checking each independently passes every current test and fails this one.
• Nullable integers. The bigger gap. The spec assigns meaning to null (offset null -> 0, count null -> ALL), so nullable integers must be accepted, and nothing currently pins that through the Fetch check.
• Non-literal expressions. Every case used a literal, but SubstraitRelVisitor deliberately passes arbitrary Calcite RexNodes through as offset/count, so a field reference and a cast belong here.
• unbound. Encodes whatever we decide in point 2; delete if we go the other way.
• assertDoesNotThrow so a failure names the offending combination instead of surfacing as a bare stack trace, and message assertions so nonIntegerCountRejected cannot pass on the offset branch firing.
• Imports. FetchRel and RelCommon can be imported; io.substrait.proto.Expression and io.substrait.proto.Rel have to stay qualified because they collide with the POJO types.
• Dropped the "so the two files stay in sync" comment, since nothing enforces it.

Suggested change

	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);
	}
	}
	package io.substrait.relation;
	import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
	import static org.junit.jupiter.api.Assertions.assertThrows;
	import static org.junit.jupiter.api.Assertions.assertTrue;
	import io.substrait.TestBase;
	import io.substrait.expression.Expression;
	import io.substrait.expression.ExpressionCreator;
	import io.substrait.proto.FetchRel;
	import io.substrait.proto.RelCommon;
	import io.substrait.type.StringTypeVisitor;
	import io.substrait.type.Type;
	import java.util.Arrays;
	import java.util.List;
	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 {
	final Rel table =
	sb.namedScan(Arrays.asList("T"), Arrays.asList("a", "b"), Arrays.asList(R.I64, R.STRING));
	/** Every integer width is acceptable, in every offset/count combination. */
	@Test
	void integerWidthsAccepted() {
	List<Expression> widths = Arrays.asList(sb.i8(1), sb.i16(1), sb.i32(1), sb.i64(1));
	for (Expression offset : widths) {
	for (Expression count : widths) {
	assertDoesNotThrow(
	() -> fetch().offset(offset).count(count).build(),
	() -> "offset " + render(offset) + " / count " + render(count));
	}
	}
	}
	/**
	* A nullable integer is accepted: the spec assigns meaning to a null offset (treated as 0) and a
	* null count (all remaining rows).
	*/
	@Test
	void nullableIntegerAccepted() {
	Expression nullOffset = ExpressionCreator.typedNull(N.I64);
	Expression nullCount = ExpressionCreator.typedNull(N.I32);
	assertDoesNotThrow(() -> fetch().offset(nullOffset).count(nullCount).build());
	}
	/** The check inspects the expression's type, so non-literal integer expressions are accepted. */
	@Test
	void nonLiteralIntegerExpressionsAccepted() {
	// Column "a" is i64. Isthmus passes arbitrary Calcite RexNodes through as offset/count, so the
	// check has to hold for more than literals.
	assertDoesNotThrow(() -> fetch().count(sb.fieldReference(table, 0)).build());
	Expression cast =
	ExpressionCreator.cast(R.I64, sb.i32(1), Expression.FailureBehavior.THROW_EXCEPTION);
	assertDoesNotThrow(() -> fetch().count(cast).build());
	}
	/**
	* An unbound offset/count is accepted: a partially bound plan may be serialized and exchanged
	* before its types are known, so the integer constraint cannot be enforced yet.
	*/
	@Test
	void unboundAccepted() {
	Expression unboundParameter =
	Expression.DynamicParameter.builder()
	.type(Type.Unbound.builder().build())
	.parameterReference(0)
	.build();
	assertDoesNotThrow(() -> fetch().offset(unboundParameter).count(unboundParameter).build());
	}
	/** A non-integer offset expression is rejected at construction time. */
	@Test
	void nonIntegerOffsetRejected() {
	IllegalArgumentException e =
	assertThrows(IllegalArgumentException.class, () -> fetch().offset(sb.fp64(1.0)).build());
	assertTrue(e.getMessage().contains("offset"), e.getMessage());
	assertTrue(e.getMessage().contains("fp64"), e.getMessage());
	}
	/** A non-integer count expression is rejected at construction time. */
	@Test
	void nonIntegerCountRejected() {
	IllegalArgumentException e =
	assertThrows(IllegalArgumentException.class, () -> fetch().count(sb.fp64(1.0)).build());
	assertTrue(e.getMessage().contains("count"), e.getMessage());
	}
	/** 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.Expression fp64Expr =
	io.substrait.proto.Expression.newBuilder()
	.setLiteral(io.substrait.proto.Expression.Literal.newBuilder().setFp64(1.0))
	.build();
	io.substrait.proto.Rel protoRel =
	io.substrait.proto.Rel.newBuilder()
	.setFetch(
	FetchRel.newBuilder()
	.setCommon(RelCommon.newBuilder())
	.setInput(relProtoConverter.toProto(table))
	.setOffsetExpr(fp64Expr))
	.build();
	assertThrows(IllegalArgumentException.class, () -> protoRelConverter.from(protoRel));
	}
	private ImmutableFetch.Builder fetch() {
	return Fetch.builder().input(table);
	}
	private static String render(Expression expression) {
	return expression.getType().accept(new StringTypeVisitor());
	}
	}

[nielspardon]

Worth pinning the two types where "not an integer" is a decision rather than an obvious fact - unbound especially, since it is what point 2 turns on.

(Separately: the two equalsIgnoringNullability tests below are unrelated to this PR. They are net-new coverage for a previously untested method so I am not going to argue for removing them, just noting they would read better as their own commit.)

Suggested change

	assertFalse(R.list(R.I64).isInteger());
	assertFalse(R.map(R.I64, R.I64).isInteger());
	}
	assertFalse(R.list(R.I64).isInteger());
	assertFalse(R.map(R.I64, R.I64).isInteger());
	}
	@Test
	void unboundAndUserDefinedAreNotIntegers() {
	assertFalse(Type.Unbound.builder().build().isInteger());
	assertFalse(R.userDefined("urn:test", "t").isInteger());
	}