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 1 commit 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.
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, or an FP round-trip that doesn't survive). It previously converted successfully but produced a type-mismatched, already spec-invalidoffset_expr.