core: interval literals and types accept component values the Substrait spec disallows

[nielspardon]

Description

The Substrait spec constrains interval literal components, and core enforces none of it. Values the spec explicitly disallows are constructible through the POJO builders and round-trip through proto unchallenged.

From the spec's type documentation (spec v0.101.0, the version pinned in gradle/libs.versions.toml):

• interval_year — "Supports a range of [-10,000..10,000] years with month precision (= [-120,000..120,000] months)", and for the literal, "int32 years and int32 months, with the added constraint that each component can never independently specify more than 10,000 years, even if the components have opposite signs". The example given as not allowed is -10000y 200000m.
• interval_day<P> — "Supports a range of [-3,650,000..3,650,000] days with fractional second precision (P, number of digits) 0 <= P <= 12". The same component rule applies, with 3650001d -86400s 0us as the disallowed example. The sub-second field "carries only the fractional part of a second, expressed in 1e(-P) units", so its magnitude belongs under 1e(P).
• interval_compound<P> inherits both sets of rules.

What core does today

Nothing validates any of it. Expression.IntervalDayLiteral declares bare int days(), int seconds(), long subseconds() and int precision() accessors; Expression.IntervalYearLiteral declares bare int years() and int months(); Expression.IntervalCompoundLiteral flattens both. None of the three carries a @Value.Check, and Type.java has no @Value.Check at all, so Type.IntervalDay.precision() is likewise unbounded. ProtoTypeConverter rejects only an unset interval-day precision, never an out-of-range one.

Three concrete gaps, in rough order of how easily they bite:

1. Per-component 10,000-year limit. The spec's own counter-examples build fine: intervalDay(false, 3650001, -86400, 0, 6) and intervalYear(false, -10000, 200000) are both accepted.
2. Sub-second magnitude. subseconds is not checked against 1e(P), so intervalDay(false, 0, 0, 3, 0) is accepted even though precision 0 leaves no room for a fractional part — and it converts to Calcite as 3 whole seconds, because the consumer has to interpret the field somehow.
3. Precision range. The spec allows 0 <= P <= 12; nothing on the type or the literal enforces either bound, so a negative precision reaches consumers. Isthmus grew its own lower-bound check for this in fix(isthmus)!: preserve interval_day precision in both conversion directions #1120, which is the sort of thing that belongs once in core rather than per-consumer.

Suggested shape

@Value.Check on each of the three literals, throwing IllegalArgumentException — not assert, which does not run in a deployed app server. The file already has the pattern: Expression.ScalarFunctionInvocation.check() delegates to a separate VariadicParameterConsistencyValidator, which keeps the rule testable on its own and out of the accessor list.

Open questions

• Breaking? Adding a check means code that currently builds an out-of-range interval starts throwing. That is the point, but it is a behaviour change for any consumer relying on the current permissiveness, so it likely wants a breaking release rather than a patch.
• Where to enforce. A @Value.Check fires on every construction, including the proto-to-POJO path, which is the strictest option. Validating only at the proto boundaries would let internal construction stay unchecked — probably the wrong trade, but worth stating.
• Total vs component. The spec bounds each component independently and gives an overall range. A component check alone still admits 3,650,000d 86400s, whose total exceeds the day range. Both checks or just the component rule the spec spells out?
• interval_compound. The spec says Substrait "gives no definition for the conversion of values between independent grains (e.g. months to days)", so a compound literal's total is not well defined and only the per-component rules can be checked.

Context

Surfaced while reviewing #1120, where scaling an interval's whole value to the declared precision overflows a long inside the spec's day range. That overflow is #1120's to fix; this issue is about core never having stated the bounds in the first place.

🤖 Generated with AI

[alexandrefimov]

Taking the open questions in order, with what I have from #1120.

Breaking — yes, and there is a decided precedent. #1058 (fix!: replace assert-based validation with real exceptions) is the same shape and shipped as breaking. Here it is concrete rather than theoretical: ProtoExpressionConverter:577 builds the literal through ExpressionCreator.intervalDay, so a @Value.Check fires on the proto import path and a plan that deserializes today stops deserializing. That is the same argument I made on #1074 for Fetch.check() — from() newly rejecting previously-importable plans is a behaviour change for consumers, whatever the plan's conformance.

Where — @Value.Check, with one split. The literal checks are uncontroversial: the values are plain int/long, there is no unbound-style escape hatch to argue about as there was on #1074, and the proto boundary is exactly where the bad values arrive. Type.IntervalDay.precision() is different in kind — Type.java has no @Value.Check at all today, so that one lands in new territory and may deserve to be decided on its own rather than bundled.

Total vs component — both, but they are not the same kind of rule. The per-component limit is what the spec spells out with counter-examples, so it is mechanical. The overall range needs a definition of "total" the spec does not give: 3,650,000d -86400s is inside it on any reading, 3,650,000d 86400s is one day outside. If total is days * 86400 + seconds in whole seconds, the arithmetic is exact in a long (3.15e11 at the maximum), so it costs nothing — it is the wording that needs care, not the code.

interval_compound — agreed, per-component only. The spec declines to define conversion between grains, so there is no total to check.

One thing worth stating so it does not look redundant later: enforcing the spec's 0 <= P <= 12 in core would not make Isthmus's own bound from #1120 unnecessary. That one is 0..9, read from getMaxScale(INTERVAL_DAY_SECOND), and it is a statement about what Calcite can carry rather than about what the spec allows. The same split is sharper on the leading field: Calcite rejects INTERVAL DAY(7) under SubstraitTypeSystem today, so a spec-maximum 3,650,000-day interval cannot be spelled in Calcite at all (details in #1124). core stating the spec's bound and Isthmus stating Calcite's is the honest arrangement — they just should not be the same number by accident.

[nielspardon]

Adding a fourth gap, adjacent to gap 3 but distinct from it. Gap 3 is about the precision range; this is about proto field presence.

Expression.Literal.PrecisionTimestamp.precision and Expression.Literal.IntervalDayToSecond.precision are plain, non-optional int32 fields in algebra.proto, so proto3 gives them no presence information — an unset field is indistinguishable from an explicit 0. ProtoExpressionConverter reads both with a bare getPrecision() and no hasPrecision() guard, and ExpressionCreator.intervalDay(nullable, days, seconds) likewise defaults precision to 0.

This is exactly the case core already guards on the type side — type.proto marks precision optional and ProtoTypeConverter throws "Interval day type must have its precision set", matching the spec's "Implementations should reject an unset precision". The literal has no counterpart, so a producer that fills subseconds in microseconds and forgets precision is read as declaring seconds precision, and any consumer that rescales by 10^(6-P) inflates the value by a millionfold rather than rejecting it.

Surfaced while reviewing #1128, where the Spark converter's exact-microsecond guard had been the de-facto backstop for this. The resolution there was to keep accepting 0..6 for literals, which is right — precision 0 is a legitimate value and the adapter has no way to see field presence — so the check belongs here instead.

Two things this adds to the issue's open questions:

• It may not be fixable at the POJO layer at all. A @Value.Check cannot distinguish unset from 0 either, because by then the value is an int. Detecting it needs hasPrecision() at the proto boundary, which argues for a guard in ProtoExpressionConverter rather than on the literal — cutting against the "enforce in @Value.Check" shape the issue proposes for the range rules. The two kinds of validation may want different homes.
• Whether to reject at all is a real question. Rejecting an unset precision makes core stricter than the wire format strictly requires, and breaks producers that legitimately mean seconds and omit the default. The type side already made that call, so consistency argues for it — but it is worth stating rather than assuming.