From c93cd7859569543d419d1174f52288302948b8c5 Mon Sep 17 00:00:00 2001 From: Anas Ismail Khan Date: Mon, 21 Sep 2026 20:46:40 +0500 Subject: [PATCH 1/4] fix(isthmus): mirror negative decimal and floating-point RANGE offsets, not just integral renamed `negate` to `negateIntegral` --- .../expression/WindowBoundConverter.java | 61 ++++++++++++++++++- .../isthmus/WindowBoundConverterTest.java | 37 +++++++++++ 2 files changed, 95 insertions(+), 3 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 3d2b7b44f..1c73a4735 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java @@ -6,6 +6,7 @@ import io.substrait.isthmus.TypeConverter; import io.substrait.type.StringTypeVisitor; import io.substrait.type.Type; +import io.substrait.util.DecimalUtil; import java.math.BigDecimal; import java.math.BigInteger; import java.util.Optional; @@ -64,10 +65,10 @@ public static WindowBound toWindowBound( // the offset: a negative offset is invalid, and the mirror bound with the magnitude is its // equivalent. Calcite only rejects a negative offset for ROWS, so RANGE reaches here. boolean preceding = rexWindowBound.isPreceding(); - Optional negative = integralValue(converted).filter(value -> value < 0); + Optional negative = negateIfNegative(converted); if (negative.isPresent()) { preceding = !preceding; - converted = negate(converted, negative.get()); + converted = negative.get(); } Expression offset = @@ -85,7 +86,43 @@ public static WindowBound toWindowBound( "window bound was none of CURRENT ROW, UNBOUNDED, PRECEDING or FOLLOWING"); } - private static Expression negate(Expression offset, long value) { + /** + * Returns the negation of {@code offset}, if it is a negative literal. + * + * @param offset the offset expression to check + * @return the negated literal, or empty if {@code offset} is not a negative literal + */ + private static Optional negateIfNegative(Expression offset) { + return decimalValue(offset) + .filter(value -> value.signum() < 0) + .map(value -> decimalLiteralOfType(offset.getType(), value.negate())) + .or( + () -> + floatingValue(offset) + .filter(value -> value < 0) + .map(value -> floatingLiteralOfType(offset.getType(), -value))) + .or( + () -> + integralValue(offset) + .filter(value -> value < 0) + .map(WindowBoundConverter::negateIntegral)); + } + + private static Optional decimalValue(Expression expression) { + if (expression instanceof Expression.DecimalLiteral) { + Expression.DecimalLiteral decimal = (Expression.DecimalLiteral) expression; + return Optional.of( + DecimalUtil.getBigDecimalFromBytes(decimal.value().toByteArray(), decimal.scale(), 16)); + } + return Optional.empty(); + } + + private static Expression decimalLiteralOfType(Type type, BigDecimal value) { + Type.Decimal decimal = (Type.Decimal) type; + return ExpressionCreator.decimal(type.nullable(), value, decimal.precision(), decimal.scale()); + } + + private static Expression negateIntegral(long value) { long negated; try { negated = Math.negateExact(value); @@ -98,6 +135,24 @@ private static Expression negate(Expression offset, long value) { return ExpressionCreator.i64(false, negated); } + private static Optional floatingValue(Expression expression) { + if (expression instanceof Expression.FP64Literal) { + return Optional.of(((Expression.FP64Literal) expression).value()); + } else if (expression instanceof Expression.FP32Literal) { + return Optional.of((double) ((Expression.FP32Literal) expression).value()); + } + return Optional.empty(); + } + + private static Expression floatingLiteralOfType(Type type, double value) { + if (type instanceof Type.FP64) { + return ExpressionCreator.fp64(type.nullable(), value); + } else if (type instanceof Type.FP32) { + return ExpressionCreator.fp32(type.nullable(), (float) value); + } + throw new IllegalStateException("expected a floating-point type, got " + type); + } + private static Expression normalizeIntegralOffset( Expression offset, boolean isRows, diff --git a/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java index 0fc63bbce..4133e751f 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java @@ -307,4 +307,41 @@ void negativeOffsetOverflowingLongIsRejectedRatherThanThrowingArithmeticExceptio WindowBoundConverter.toWindowBound( bound, false, Optional.of(orderingType), rexExpressionConverter)); } + + @Test + void negativeDecimalPrecedingOffsetIsFlippedToFollowingWithItsMagnitude() { + // The integral mirror above has a decimal counterpart: RANGE BETWEEN -5.5 PRECEDING is + // equivalent to FOLLOWING 5.5. + RexNode offset = c(new BigDecimal("-5.5"), SqlTypeName.DECIMAL, 19, 1); + RexWindowBound bound = RexWindowBounds.preceding(offset); + + WindowBound converted = + WindowBoundConverter.toWindowBound(bound, false, Optional.empty(), rexExpressionConverter); + + assertEquals( + WindowBound.Following.of(ExpressionCreator.decimal(false, new BigDecimal("5.5"), 19, 1)), + converted); + } + + @Test + void negativeDoublePrecedingOffsetIsFlippedToFollowingWithItsMagnitude() { + RexNode offset = c(-5.5, SqlTypeName.DOUBLE); + RexWindowBound bound = RexWindowBounds.preceding(offset); + + WindowBound converted = + WindowBoundConverter.toWindowBound(bound, false, Optional.empty(), rexExpressionConverter); + + assertEquals(WindowBound.Following.of(ExpressionCreator.fp64(false, 5.5)), converted); + } + + @Test + void negativeRealPrecedingOffsetIsFlippedToFollowingWithItsMagnitude() { + RexNode offset = c(-5.5f, SqlTypeName.REAL); + RexWindowBound bound = RexWindowBounds.preceding(offset); + + WindowBound converted = + WindowBoundConverter.toWindowBound(bound, false, Optional.empty(), rexExpressionConverter); + + assertEquals(WindowBound.Following.of(ExpressionCreator.fp32(false, 5.5f)), converted); + } } From b83ff76de1038ae362a6eb4bee0a57ac98fc6c84 Mon Sep 17 00:00:00 2001 From: Anas Ismail Khan Date: Mon, 21 Sep 2026 20:56:54 +0500 Subject: [PATCH 2/4] zero float/double/decimal offset becomes `CURRENT_ROW` --- .../expression/WindowBoundConverter.java | 15 +++++++- .../isthmus/WindowBoundConverterTest.java | 35 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) 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 1c73a4735..885d8cff1 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java @@ -57,7 +57,7 @@ public static WindowBound toWindowBound( // 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()) { + if (isZero(converted)) { return WindowBound.CURRENT_ROW; } @@ -86,6 +86,19 @@ public static WindowBound toWindowBound( "window bound was none of CURRENT ROW, UNBOUNDED, PRECEDING or FOLLOWING"); } + /** + * Reports whether {@code offset} is a decimal, floating-point, or integral literal with a value + * of zero. + * + * @param offset the offset expression to check + * @return {@code true} if {@code offset} is a zero-valued literal + */ + private static boolean isZero(Expression offset) { + return decimalValue(offset).map(value -> value.signum() == 0).orElse(false) + || floatingValue(offset).map(value -> value == 0).orElse(false) + || integralValue(offset).map(value -> value == 0).orElse(false); + } + /** * Returns the negation of {@code offset}, if it is a negative literal. * diff --git a/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java index 4133e751f..180f595f6 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java @@ -250,6 +250,41 @@ void zeroOffsetBecomesCurrentRowEvenAgainstAnUnsupportedOrderingType() { assertEquals(WindowBound.CURRENT_ROW, converted); } + @Test + void zeroDecimalOffsetBecomesCurrentRow() { + // The integral zero check above has a decimal counterpart: a zero-valued DecimalLiteral + // needs no representation in the ordering expression's type either. + RexNode offset = c(BigDecimal.ZERO, SqlTypeName.DECIMAL, 19, 1); + RexWindowBound bound = RexWindowBounds.preceding(offset); + + WindowBound converted = + WindowBoundConverter.toWindowBound(bound, false, Optional.empty(), rexExpressionConverter); + + assertEquals(WindowBound.CURRENT_ROW, converted); + } + + @Test + void zeroDoubleOffsetBecomesCurrentRow() { + RexNode offset = c(0.0, SqlTypeName.DOUBLE); + RexWindowBound bound = RexWindowBounds.preceding(offset); + + WindowBound converted = + WindowBoundConverter.toWindowBound(bound, false, Optional.empty(), rexExpressionConverter); + + assertEquals(WindowBound.CURRENT_ROW, converted); + } + + @Test + void zeroRealOffsetBecomesCurrentRow() { + RexNode offset = c(0.0f, SqlTypeName.REAL); + RexWindowBound bound = RexWindowBounds.preceding(offset); + + WindowBound converted = + WindowBoundConverter.toWindowBound(bound, false, Optional.empty(), rexExpressionConverter); + + assertEquals(WindowBound.CURRENT_ROW, converted); + } + @Test void negativePrecedingOffsetIsFlippedToFollowingWithItsMagnitude() { // The spec carries a bound's direction in the Preceding/Following choice, not in the sign of From e4d5d7611024c4c5ecfe001623fc0c37131f02f8 Mon Sep 17 00:00:00 2001 From: Anas Ismail Khan Date: Tue, 22 Sep 2026 13:43:58 +0500 Subject: [PATCH 3/4] inline the negations and add more test coverage --- .../expression/WindowBoundConverter.java | 115 ++++++------------ .../isthmus/WindowBoundConverterTest.java | 54 ++++++++ 2 files changed, 91 insertions(+), 78 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 885d8cff1..8f5151d78 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java @@ -53,19 +53,45 @@ public static WindowBound toWindowBound( RexNode node = rexWindowBound.getOffset(); Expression converted = node.accept(rexExpressionConverter); + boolean preceding = rexWindowBound.isPreceding(); - // 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 (isZero(converted)) { - return WindowBound.CURRENT_ROW; + // Per the spec, zero is not a valid offset (equivalent to CurrentRow) and a negative offset is + // invalid (the mirror bound with the magnitude is its equivalent); Calcite only rejects a + // negative offset for ROWS, so RANGE reaches here. Checked in this order because a -0.0 + // offset compares equal to zero but not less than zero. + Optional negative; + if (converted instanceof Expression.DecimalLiteral) { + Expression.DecimalLiteral literal = (Expression.DecimalLiteral) converted; + BigDecimal value = + DecimalUtil.getBigDecimalFromBytes(literal.value().toByteArray(), literal.scale(), 16); + if (value.signum() == 0) { + return WindowBound.CURRENT_ROW; + } + negative = + value.signum() < 0 + ? Optional.of( + ExpressionCreator.decimal( + false, value.negate(), literal.precision(), literal.scale())) + : Optional.empty(); + } else if (converted instanceof Expression.FP64Literal) { + double value = ((Expression.FP64Literal) converted).value(); + if (value == 0) { + return WindowBound.CURRENT_ROW; + } + negative = value < 0 ? Optional.of(ExpressionCreator.fp64(false, -value)) : Optional.empty(); + } else if (converted instanceof Expression.FP32Literal) { + float value = ((Expression.FP32Literal) converted).value(); + if (value == 0) { + return WindowBound.CURRENT_ROW; + } + negative = value < 0 ? Optional.of(ExpressionCreator.fp32(false, -value)) : Optional.empty(); + } else { + Optional value = integralValue(converted); + if (value.filter(v -> v == 0).isPresent()) { + return WindowBound.CURRENT_ROW; + } + negative = value.filter(v -> v < 0).map(WindowBoundConverter::negateIntegral); } - - // The spec carries a bound's direction in the Preceding/Following choice, not in the sign of - // the offset: a negative offset is invalid, and the mirror bound with the magnitude is its - // equivalent. Calcite only rejects a negative offset for ROWS, so RANGE reaches here. - boolean preceding = rexWindowBound.isPreceding(); - Optional negative = negateIfNegative(converted); if (negative.isPresent()) { preceding = !preceding; converted = negative.get(); @@ -86,55 +112,6 @@ public static WindowBound toWindowBound( "window bound was none of CURRENT ROW, UNBOUNDED, PRECEDING or FOLLOWING"); } - /** - * Reports whether {@code offset} is a decimal, floating-point, or integral literal with a value - * of zero. - * - * @param offset the offset expression to check - * @return {@code true} if {@code offset} is a zero-valued literal - */ - private static boolean isZero(Expression offset) { - return decimalValue(offset).map(value -> value.signum() == 0).orElse(false) - || floatingValue(offset).map(value -> value == 0).orElse(false) - || integralValue(offset).map(value -> value == 0).orElse(false); - } - - /** - * Returns the negation of {@code offset}, if it is a negative literal. - * - * @param offset the offset expression to check - * @return the negated literal, or empty if {@code offset} is not a negative literal - */ - private static Optional negateIfNegative(Expression offset) { - return decimalValue(offset) - .filter(value -> value.signum() < 0) - .map(value -> decimalLiteralOfType(offset.getType(), value.negate())) - .or( - () -> - floatingValue(offset) - .filter(value -> value < 0) - .map(value -> floatingLiteralOfType(offset.getType(), -value))) - .or( - () -> - integralValue(offset) - .filter(value -> value < 0) - .map(WindowBoundConverter::negateIntegral)); - } - - private static Optional decimalValue(Expression expression) { - if (expression instanceof Expression.DecimalLiteral) { - Expression.DecimalLiteral decimal = (Expression.DecimalLiteral) expression; - return Optional.of( - DecimalUtil.getBigDecimalFromBytes(decimal.value().toByteArray(), decimal.scale(), 16)); - } - return Optional.empty(); - } - - private static Expression decimalLiteralOfType(Type type, BigDecimal value) { - Type.Decimal decimal = (Type.Decimal) type; - return ExpressionCreator.decimal(type.nullable(), value, decimal.precision(), decimal.scale()); - } - private static Expression negateIntegral(long value) { long negated; try { @@ -148,24 +125,6 @@ private static Expression negateIntegral(long value) { return ExpressionCreator.i64(false, negated); } - private static Optional floatingValue(Expression expression) { - if (expression instanceof Expression.FP64Literal) { - return Optional.of(((Expression.FP64Literal) expression).value()); - } else if (expression instanceof Expression.FP32Literal) { - return Optional.of((double) ((Expression.FP32Literal) expression).value()); - } - return Optional.empty(); - } - - private static Expression floatingLiteralOfType(Type type, double value) { - if (type instanceof Type.FP64) { - return ExpressionCreator.fp64(type.nullable(), value); - } else if (type instanceof Type.FP32) { - return ExpressionCreator.fp32(type.nullable(), (float) value); - } - throw new IllegalStateException("expected a floating-point type, got " + type); - } - private static Expression normalizeIntegralOffset( Expression offset, boolean isRows, diff --git a/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java index 180f595f6..a4f138603 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/WindowBoundConverterTest.java @@ -358,6 +358,60 @@ void negativeDecimalPrecedingOffsetIsFlippedToFollowingWithItsMagnitude() { converted); } + @Test + void negativeDecimalFollowingOffsetIsFlippedToPrecedingWithItsMagnitude() { + RexNode offset = c(new BigDecimal("-5.5"), SqlTypeName.DECIMAL, 19, 1); + RexWindowBound bound = RexWindowBounds.following(offset); + + WindowBound converted = + WindowBoundConverter.toWindowBound(bound, false, Optional.empty(), rexExpressionConverter); + + assertEquals( + WindowBound.Preceding.of(ExpressionCreator.decimal(false, new BigDecimal("5.5"), 19, 1)), + converted); + } + + @Test + void negativeDecimalPrecedingOffsetIsNotRetypedAgainstTheOrderingType() { + // Unlike the integral mirror, a decimal offset's magnitude is not retyped against the + // ordering column: normalizeIntegralOffset only consults integralValue, so the mirrored + // decimal literal keeps its own precision and scale regardless of orderingType. + RexNode offset = c(new BigDecimal("-5.5"), SqlTypeName.DECIMAL, 19, 1); + RexWindowBound bound = RexWindowBounds.preceding(offset); + RelDataType orderingType = t(SqlTypeName.DECIMAL, 5, 2); + + WindowBound converted = + WindowBoundConverter.toWindowBound( + bound, false, Optional.of(orderingType), rexExpressionConverter); + + assertEquals( + WindowBound.Following.of(ExpressionCreator.decimal(false, new BigDecimal("5.5"), 19, 1)), + converted); + } + + @Test + void precedingWithPositiveDoubleOffsetIsUnchanged() { + // A positive FP offset must not be mistaken for negative or zero by isZero/negation. + RexNode offset = c(5.5, SqlTypeName.DOUBLE); + RexWindowBound bound = RexWindowBounds.preceding(offset); + + WindowBound converted = + WindowBoundConverter.toWindowBound(bound, false, Optional.empty(), rexExpressionConverter); + + assertEquals(WindowBound.Preceding.of(ExpressionCreator.fp64(false, 5.5)), converted); + } + + @Test + void precedingWithPositiveRealOffsetIsUnchanged() { + RexNode offset = c(5.5f, SqlTypeName.REAL); + RexWindowBound bound = RexWindowBounds.preceding(offset); + + WindowBound converted = + WindowBoundConverter.toWindowBound(bound, false, Optional.empty(), rexExpressionConverter); + + assertEquals(WindowBound.Preceding.of(ExpressionCreator.fp32(false, 5.5f)), converted); + } + @Test void negativeDoublePrecedingOffsetIsFlippedToFollowingWithItsMagnitude() { RexNode offset = c(-5.5, SqlTypeName.DOUBLE); From 5be4125f4bc68a65707bac00cabbb0c9490e5bd1 Mon Sep 17 00:00:00 2001 From: Anas Ismail Khan Date: Tue, 22 Sep 2026 18:44:15 +0500 Subject: [PATCH 4/4] remove "integral" from class javadoc --- .../io/substrait/isthmus/expression/WindowBoundConverter.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 8f5151d78..64b3eed74 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/WindowBoundConverter.java @@ -19,8 +19,8 @@ * *

Supports {@code CURRENT ROW}, {@code UNBOUNDED}, and {@code PRECEDING}/{@code FOLLOWING} * bounds with an arbitrary offset expression. A RANGE bound's integral literal offset must match - * the ordering expression's exact type. A negative integral offset is mirrored to the opposite - * bound with its magnitude, and a zero offset becomes {@code CURRENT ROW}. + * the ordering expression's exact type. A negative offset is mirrored to the opposite bound with + * its magnitude, and a zero offset becomes {@code CURRENT ROW}. */ public class WindowBoundConverter {