fix(isthmus)!: reject RANGE offsets that cannot be retyped to the ordering column - #1206
fix(isthmus)!: reject RANGE offsets that cannot be retyped to the ordering column#1206anasik wants to merge 8 commits into
Conversation
alexandrefimov
left a comment
There was a problem hiding this comment.
A zero offset stops converting where the ordering type has no integral form. toWindowBound normalizes before it checks for zero, so the new refusal fires first. Measured on this branch against its parent (8088c0d): RexWindowBounds.preceding(0) with a TIMESTAMP, DATE or VARCHAR ordering type returned CurrentRow before and throws now, while SMALLINT gives CurrentRow either way. Zero needs no retyping — the spec makes it equivalent to CURRENT ROW, which toWindowBound does two lines below the change. zeroOffsetBecomesCurrentRow stays green because it converts a ROWS bound with no ordering type, so it never reaches the new branch.
Reading the zero check before normalizing keeps both:
RexNode node = rexWindowBound.getOffset();
Expression converted = node.accept(rexExpressionConverter);
// Per the spec, zero is not a valid offset; it is equivalent to CurrentRow, and producers
// should emit CurrentRow rather than a zero offset_expr.
if (integralValue(converted).filter(value -> value == 0).isPresent()) {
return WindowBound.CURRENT_ROW;
}
Expression offset =
normalizeIntegralOffset(
converted, isRows, orderingType, rexExpressionConverter.getTypeConverter());With that, preceding(5) over a TIMESTAMP ordering still throws, the four new tests pass, and so does the rest of :isthmus:test.
Two smaller things. An interval offset leaves normalizeIntegralOffset before the ordering type is consulted, so a temporal ordering column with one converts exactly as before — checked on TIMESTAMP and DATE, both still give back the IntervalDayLiteral unchanged, so the change does not reach RANGE INTERVAL ... PRECEDING. And the BREAKING CHANGE note names three causes — out of range, past a decimal's precision, an FP round trip that does not survive — where there is a fourth: an ordering type with no integral form at all, which is what rangeOffsetAgainstUnsupportedOrderingTypeThrows covers and what a temporal ordering column runs into. Maybe worth naming it there, since it is the one a reader is most likely to meet.
nielspardon
left a comment
There was a problem hiding this comment.
Move the zero-offset check above normalizeIntegralOffset: RANGE BETWEEN 0 PRECEDING over a DECIMAL(5,5) ordering column returns CurrentRow on main and throws on this branch, and the same reordering also stops the existing Preceding{0.00} emission for decimal and FP ordering columns that the spec tells producers not to write.
Separately, the add(T, D) -> T framing needs a second look. The spec asks only that D be compatible with T (and requires subtract(T, D) -> T as well), and for decimals add always widens the precision — so no decimal offset ever literally satisfies the rule the new message cites, including the ones the code accepts. Since #1199 left reject-vs-widen open and the spec says nothing about what a producer should do when it cannot represent the offset, it would help to state in the PR which option you chose and why.
One thing to fix before merge because it cannot be corrected after a release: the BREAKING CHANGE: footer is the only part of the body published verbatim, and it currently ends `offset_expr.` with the period inside the code span. It also omits the fourth rejection cause — any ordering type integralLiteralOfType has no case for — and the commit title is missing the ! that the PR title carries.
1a78d7e to
31d8433
Compare
alexandrefimov
left a comment
There was a problem hiding this comment.
Measured on the merge head (00d510d): a DATE or VARCHAR ordering — the two the new tests don't cover — is back to CurrentRow at zero, a non-zero offset over either still throws, and SMALLINT retypes as before.
The reorder can't drop a zero the old placement caught: normalizeIntegralOffset maps an integral literal to an integral literal of the same value, so integralValue sees the same number on either side of it — including on the ROWS path, where zero used to arrive as i64(0).
nielspardon
left a comment
There was a problem hiding this comment.
RANGE BETWEEN -5 PRECEDING still converts to Preceding{I32Literal{-5}} from plain SQL here, and it's the same spec sentence this PR implements: site/docs/expressions/window_functions.md:34 at the pinned v0.102.0 requires "a non-negative distance whose type D is compatible with the ordering expression's type T", and algebra.proto gives the remedy on Preceding.offset — "Use CurrentRow for offset zero and Following for negative offsets." So it parallels the zero case you just hoisted: negate and flip to the mirror bound rather than throw, which keeps isthmus accepting what Calcite accepts (Calcite validates the sign for ROWS only) while emitting a valid plan. Whether that belongs here or in a follow-up is your call; the other three comments are smaller. Filed #1229 and #1230 for two adjacent gaps that predate this PR — nothing needed here for either.
…cking both the offset value and a distinguishing type-name substring, so a wrong-branch throw can't pass silently) - Added rangeOffsetAcceptsTheOrderingTypesUpperBoundButNotBeyondIt — boundary test proving Short.MAX_VALUE retypes cleanly while Short.MAX_VALUE + 1 throws - Added rangeOffsetAgainstDateOrderingTypeThrows — a second "no such case" test so TIMESTAMP isn't the only thing covering that path
BREAKING CHANGE:
SqlToSubstraitnow throwsUnsupportedOperationExceptionwhen aRANGEwindow's integral offset cannot be retyped to the ordering column's type (out of range, past a decimal's precision, an FP round-trip that doesn't survive, or an ordering type with no integral form at all — the one a temporal ordering column hits). It previously converted successfully but produced a type-mismatched, already spec-invalidoffset_expr.Closes #1199