From 84a75f200fe58303862333c93be5c2235f59a493 Mon Sep 17 00:00:00 2001 From: Anas Ismail Khan Date: Mon, 31 Aug 2026 16:36:20 +0500 Subject: [PATCH 1/2] fix(isthmus)!: reject RANGE offsets that cannot be retyped to the ordering column --- .../expression/WindowBoundConverter.java | 31 ++++++--- .../isthmus/WindowBoundConverterTest.java | 63 ++++++++++++++++++- 2 files changed, 82 insertions(+), 12 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java index c34b3cb99..1ebe2c5b3 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java @@ -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 does not fit the + * ordering expression's exact type */ public static WindowBound toWindowBound( RexWindowBound rexWindowBound, @@ -45,19 +47,19 @@ public static WindowBound toWindowBound( } RexNode node = rexWindowBound.getOffset(); - Expression offset = - normalizeIntegralOffset( - node.accept(rexExpressionConverter), - isRows, - orderingType, - rexExpressionConverter.getTypeConverter()); + 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(offset).filter(value -> value == 0).isPresent()) { + // 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()); + if (rexWindowBound.isPreceding()) { return WindowBound.Preceding.of(offset); } @@ -82,10 +84,19 @@ 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: an exact type match is isthmus's own policy, not a spec mandate. return orderingType .map(typeConverter::toSubstrait) - .flatMap(type -> integralLiteralOfType(type, value.get())) + .map( + type -> + integralLiteralOfType(type, value.get()) + .orElseThrow( + () -> + new UnsupportedOperationException( + "RANGE window offset " + + value.get() + + " does not fit the ordering expression's type " + + type))) .orElse(offset); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java index 7633adddb..18700d062 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java @@ -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; @@ -83,8 +84,8 @@ void rowsIntegralOffsetIsWidenedToI64() { @Test void rangeIntegralOffsetTakesTheOrderingExpressionType() { - // Per the spec, a RANGE offset's type D must keep add(T, D) -> T defined for the ordering - // expression's type T -- forcing it to int64 would break that for, e.g., an i32 column. + // isthmus requires a RANGE offset's type to exactly match the ordering expression's type T -- + // forcing it to int64 would break that for, e.g., an i32 column. RexNode offset = c(5, SqlTypeName.INTEGER); RexWindowBound bound = RexWindowBounds.preceding(offset); RelDataType orderingType = t(SqlTypeName.INTEGER); @@ -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 no non-zero offset can + // ever be retyped to it. + 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 From 31d8433583c1e41ea6fceb113c3ea2046941b1be Mon Sep 17 00:00:00 2001 From: Anas Ismail Khan Date: Wed, 2 Sep 2026 21:43:49 +0500 Subject: [PATCH 2/2] fix(isthmus)!: reject RANGE offsets that cannot be retyped to the ordering column --- .../isthmus/WindowBoundConverterTest.java | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java index 18700d062..af129a054 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java @@ -167,4 +167,35 @@ void zeroOffsetBecomesCurrentRow() { assertEquals(WindowBound.CURRENT_ROW, converted); } + + @Test + void zeroOffsetBecomesCurrentRowEvenWhenItWouldNotFitTheDecimalOrderingType() { + // Regression test: a zero offset must short-circuit to CurrentRow before retyping is + // attempted. digitCount(0) is 1, so retyping 0 against DECIMAL(5,5) would otherwise throw + // (1 + scale(5) > precision(5)), even though zero always needs no representation at all. + RexNode offset = c(0, SqlTypeName.INTEGER); + RexWindowBound bound = RexWindowBounds.preceding(offset); + RelDataType orderingType = t(SqlTypeName.DECIMAL, 5, 5); + + WindowBound converted = + WindowBoundConverter.toWindowBound( + bound, false, Optional.of(orderingType), rexExpressionConverter); + + assertEquals(WindowBound.CURRENT_ROW, converted); + } + + @Test + void zeroOffsetBecomesCurrentRowEvenAgainstAnUnsupportedOrderingType() { + // Regression test: integralLiteralOfType has no case for TIMESTAMP, so retyping a zero offset + // against it would otherwise throw, even though zero always needs no representation at all. + RexNode offset = c(0, SqlTypeName.INTEGER); + RexWindowBound bound = RexWindowBounds.preceding(offset); + RelDataType orderingType = t(SqlTypeName.TIMESTAMP); + + WindowBound converted = + WindowBoundConverter.toWindowBound( + bound, false, Optional.of(orderingType), rexExpressionConverter); + + assertEquals(WindowBound.CURRENT_ROW, converted); + } }