core: RANGE window frames accept sorts and offset types the spec disallows

[nielspardon]

Description

Spec v0.102.0 states three rules for a RANGE window frame whose bounds are Preceding/Following. core enforces one of them (bounds_type must be set — added in #1156 as a @Value.Check on both invocation types) and none of the other two. Plans that violate them are constructible through the POJO builders and round-trip through proto unchallenged.

From algebra.proto at v0.102.0, on bounds_type (the same paragraph appears on Expression.WindowFunction and on ConsistentPartitionWindowRel.WindowRelFunction):

For RANGE with a Preceding or Following bound, sorts on the enclosing ConsistentPartitionWindowRel must contain exactly one ordering expression and must not use SORT_DIRECTION_CLUSTERED or a custom comparison function.

BOUNDS_TYPE_RANGE's own comment repeats the first half — "This bounds type requires exactly one ordering expression if Preceding or Following is used" — and the offset_expr comment adds the type rule:

Its type D must be compatible with the type T of the single ordering expression, i.e. add(T, D) -> T and subtract(T, D) -> T must be defined (e.g. timestamp/interval_day, decimal/decimal, i64/i64).

What core does today

Nothing checks any of the three. Concretely:

1. Exactly one ordering expression. A RANGE frame with zero sorts or with two is accepted. Both shapes were in feat(core)!: support offset_expr on window bounds (spec v0.102.0) #1156's own test fixtures before review caught them, which is a fair sign of how easily this is missed.
2. Sort direction / comparison function. SortDirection includes CLUSTERED, and nothing excludes it from a RANGE frame's sorts. The "custom comparison function" half is currently moot for a different reason: Expression.SortField models only expr() and direction(), so the proto's comparison_function_reference has no POJO representation at all — worth a separate issue if that's news.
3. Offset/ordering type compatibility. add(T, D) -> T is unchecked, so an i64 offset against an i32 or decimal ordering column is accepted. isthmus's producer side now derives the offset type from the ordering column (feat(core)!: support offset_expr on window bounds (spec v0.102.0) #1156), but a plan built directly, or read from proto, has no such guarantee.

Suggested shape

The two window kinds need the check in different places, which is the interesting part:

• Expression.WindowFunctionInvocation carries sort(), the bounds, and boundsType() itself, so its existing @Value.Check can cover this.
• ConsistentPartitionWindow keeps getSorts() on the relation while boundsType()/bounds live on each WindowRelFunctionInvocation, so the invocation-level check added in feat(core)!: support offset_expr on window bounds (spec v0.102.0) #1156 structurally cannot see the sorts. The sort-count and direction rules have to be a @Value.Check on ConsistentPartitionWindow iterating getWindowFunctions().

IllegalArgumentException, not assert — see the reasoning in #1047.

Open questions

• Type compatibility is a different weight class. Checking add(T, D) -> T means resolving a function against an extension collection from inside POJO validation, which the model does not otherwise do. Options: check only the easy structural rules here and leave type compatibility to a plan-level validator, approximate it with a same-type-family rule, or skip it entirely and document the gap.
• Breaking? Same trade as core: interval literals and types accept component values the Substrait spec disallows #1129: code that builds an invalid RANGE frame today starts throwing, which is the point but is a behaviour change.
• CLUSTERED reachability. Worth confirming whether any producer in the repo can currently emit a CLUSTERED sort under a RANGE frame, since that would make the check a live fix rather than a guard.

Context

Surfaced while reviewing #1156, which added the bounds_type rule from this same spec paragraph and left the rest. Two of that PR's RANGE fixtures were spec-invalid until review; they were fixed, but only by hand, since nothing fails. #1199 is the isthmus-side counterpart: its producer now derives the offset type from the ordering column, but falls back silently to a mismatched one when it cannot.

🤖 Generated with AI

[anasik]

@nielspardon taking this