Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anasik wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ public class WindowBoundConverter {
* @return the corresponding Substrait {@link WindowBound}
* @throws IllegalStateException if the bound is not one of CURRENT ROW, UNBOUNDED, PRECEDING, or
* FOLLOWING
* @throws UnsupportedOperationException if a RANGE offset's integral literal cannot be retyped to
* the ordering expression's type without breaking {@code add(T, D) -> T}
*/
public static WindowBound toWindowBound(
RexWindowBound rexWindowBound,
Expand Down Expand Up @@ -82,10 +84,21 @@ private static Expression normalizeIntegralOffset(
// The spec requires a BOUNDS_TYPE_ROWS offset_expr to be int64.
return ExpressionCreator.i64(false, value.get());
}
// 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

@nielspardon nielspardon Sep 1, 2026

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.

.map(typeConverter::toSubstrait)
.flatMap(type -> integralLiteralOfType(type, value.get()))
.map(
type ->
integralLiteralOfType(type, value.get())
.orElseThrow(
() ->
new UnsupportedOperationException(

@nielspardon nielspardon Sep 1, 2026

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.

"RANGE window offset "
+ value.get()
+ " cannot be represented as the ordering expression's type "
+ type
+ " without breaking add(T, D) -> T")))
.orElse(offset);
}

Expand Down
59 changes: 59 additions & 0 deletions isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.substrait.isthmus;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import io.substrait.expression.Expression;
import io.substrait.expression.ExpressionCreator;
Expand Down Expand Up @@ -96,6 +97,64 @@ void rangeIntegralOffsetTakesTheOrderingExpressionType() {
assertEquals(WindowBound.Preceding.of(ExpressionCreator.i32(false, 5)), converted);
}

@Test
void rangeOffsetOutOfRangeForOrderingTypeThrows() {
// Calcite's SqlWindow#validateFrameBoundary only checks the bound's type family against the
// ordering type for RANGE, not its range, so an offset that doesn't fit the ordering column's
// narrower type must be rejected here rather than silently kept as the literal's own type.
RexNode offset = c(100000, SqlTypeName.INTEGER);
RexWindowBound bound = RexWindowBounds.preceding(offset);
RelDataType orderingType = t(SqlTypeName.SMALLINT);

assertThrows(
UnsupportedOperationException.class,
() ->
WindowBoundConverter.toWindowBound(
bound, false, Optional.of(orderingType), rexExpressionConverter));
}

@Test
void rangeOffsetExceedingDecimalPrecisionThrows() {
RexNode offset = c(12345, SqlTypeName.INTEGER);
RexWindowBound bound = RexWindowBounds.preceding(offset);
RelDataType orderingType = t(SqlTypeName.DECIMAL, 5, 2);

assertThrows(
UnsupportedOperationException.class,
() ->
WindowBoundConverter.toWindowBound(
bound, false, Optional.of(orderingType), rexExpressionConverter));
}

@Test
void rangeOffsetFailingFloatRoundTripThrows() {
// 16_777_217 (2^24 + 1) is the first integer a 24-bit float mantissa cannot represent exactly.
RexNode offset = c(16777217, SqlTypeName.INTEGER);
RexWindowBound bound = RexWindowBounds.preceding(offset);
RelDataType orderingType = t(SqlTypeName.REAL);

assertThrows(
UnsupportedOperationException.class,
() ->
WindowBoundConverter.toWindowBound(
bound, false, Optional.of(orderingType), rexExpressionConverter));
}

@Test
void rangeOffsetAgainstUnsupportedOrderingTypeThrows() {
// integralLiteralOfType has no case for a temporal ordering column, so add(T, D) -> T can
// never be satisfied here regardless of the offset's value.
RexNode offset = c(5, SqlTypeName.INTEGER);
RexWindowBound bound = RexWindowBounds.preceding(offset);
RelDataType orderingType = t(SqlTypeName.TIMESTAMP);

assertThrows(
UnsupportedOperationException.class,
() ->
WindowBoundConverter.toWindowBound(
bound, false, Optional.of(orderingType), rexExpressionConverter));
}

@Test
void zeroOffsetBecomesCurrentRow() {
// Per the spec, zero is not a valid offset and is equivalent to CurrentRow; producers should
Expand Down
Loading