Skip to content

fix(isthmus)!: reject RANGE offsets that cannot be retyped to the ordering column - #1206

Open
anasik wants to merge 1 commit into
substrait-io:mainfrom
anasik:isthmus-range-offset-retype
Open

fix(isthmus)!: reject RANGE offsets that cannot be retyped to the ordering column#1206
anasik wants to merge 1 commit into
substrait-io:mainfrom
anasik:isthmus-range-offset-retype

Conversation

@anasik

@anasik anasik commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

BREAKING CHANGE: SqlToSubstrait now throws UnsupportedOperationException when a RANGE window'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-invalid offset_expr.

@alexandrefimov alexandrefimov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nielspardon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// BOUNDS_TYPE_RANGE: keep add(T, D) -> T defined for the ordering expression's type T.
// BOUNDS_TYPE_RANGE: keep add(T, D) -> T defined for the ordering expression's type T. If the
// ordering type is unknown, there is nothing to retype against, so the literal is left as-is.
return orderingType

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for a zero offset before retyping — a zero offset needs no representation in the ordering expression's type, so it should never reach this guard. On this branch order by d55 range between 0 preceding (DECIMAL(5,5)) throws where main returns CurrentRow, because digitCount(0) is 1 and 1 + 5 > 5; the same move also turns the current Preceding{0} output for DECIMAL(10,2) and DOUBLE ordering columns into the CurrentRow the spec asks for.

In toWindowBound, replacing lines 49-61 with:

    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. Checked before retyping: a zero
    // offset needs no representation in the ordering expression's type.
    if (integralValue(converted).filter(value -> value == 0).isPresent()) {
      return WindowBound.CURRENT_ROW;
    }

    Expression offset =
        normalizeIntegralOffset(
            converted, isRows, orderingType, rexExpressionConverter.getTypeConverter());

keeps the rejection this PR is adding (100000 over a SMALLINT column still throws) and leaves WindowBoundConverterTest and WindowFunctionTest green.

integralLiteralOfType(type, value.get())
.orElseThrow(
() ->
new UnsupportedOperationException(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reword this to say what isthmus cannot build rather than what the spec forbids. functions_arithmetic_decimal.yaml derives add(decimal<P1,S1>, decimal<P2,S2>) with init_prec = init_scale + max(P1-S1, P2-S2) + 1, so add(T, D) -> T holds for no decimal pair at all — not even decimal(5,2) with itself, which this method accepts. The spec's own examples (timestamp/interval_day, decimal/decimal) show D need not equal T, and a wider decimal offset would satisfy that pairing here, so requiring D == T is an isthmus policy rather than a spec verdict.

The @throws at line 34 and the comment at line 87 carry the same claim, and both omit the subtract(T, D) -> T half of the requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

isthmus: a window offset that cannot be retyped to the ordering column falls back to a type-mismatched offset

3 participants