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]

Should unbound be exempt? Type.Unbound isn't an integer, so a DynamicParameter offset — or a field reference into an unbound column — now throws at construction. And since ProtoRelConverter.from() builds the POJO, such a plan fails to import, not merely to build locally. Type.Unbound's own Javadoc says a partially bound plan "may be serialized and exchanged, but it must be bound to concrete types before execution", which is exactly what an import-time type check can't honour. This is also the first rel-level validation that inspects an expression's type, so it sets the precedent for later ones. My inline suggestion exempts it, with the one-line revert noted there.

Three smaller ones:

• TopN has the same two fields and the same spec wording, and is reachable via ProtoRelConverter.newTopN — validating one and not the other half-enforces the invariant. The same check with the message prefixed TopN compiles and keeps the suite green.
• FetchRoundtripTest only ever uses i64. One case would pin the widened acceptance through proto conversion: offset(sb.i8(3)).count(sb.i32(7)).
• from() now throws on plans it previously accepted, and the check isn't overridable, so consumers get no leniency seam. Worth deciding deliberately whether that carries a literal BREAKING CHANGE: footer — a heading alone produces no release-notes text.

Not for this PR: the spec's "evaluating to a negative integer should result in an error" is statically checkable for literals.

Suggestions below are all compiled against your head commit.

[nielspardon]

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

For discussion: exempting unbound (see summary). If you'd rather not, drop && !(type instanceof Type.Unbound) and the second Javadoc paragraph — everything else stands on its own.

The rest: Javadoc, which -Xdoclint:all -Xwerror requires on a protected member; collapsing the duplicated branches; and rendering the type through StringTypeVisitor, so the message reads decimal<10,2> instead of leaking Immutables toString (today it really does say but got: Unbound{}).

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, so a helper comparing the two types to each other rather than checking each independently would pass every current test and fail this one.
• Nullable integers - the bigger gap: the spec assigns meaning to null (offset null -> 0, count null -> ALL), and nothing pins that through the check today.
• Non-literal expressions - SubstraitRelVisitor deliberately passes arbitrary Calcite RexNodes through as offset/count, so a field reference and a cast belong here.
• unbound - encodes whatever we settle in the summary; delete if we go the other way.
• assertDoesNotThrow + message assertions, so a failure names the offending combination and nonIntegerCountRejected cannot pass on the offset branch firing.

FetchRel and RelCommon are importable; io.substrait.proto.Expression and io.substrait.proto.Rel have to stay qualified - they collide with the POJO types.

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 the summary's unbound question 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());
	}