From 2a893a619f38179586cab854c1ed56b4c0fff646 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 18 Aug 2026 22:33:41 +0300 Subject: [PATCH 1/2] fix(isthmus): convert pre-epoch temporal literals instead of throwing getTimestampString and createTimeString split a value into whole seconds and a nanosecond remainder with truncating division. Before the epoch the value is negative, so truncation leaves a negative remainder, and both TimestampString and TimeString reject that with a bare IllegalArgumentException: a precision_timestamp of -500 at precision 3, which is 1969-12-31 23:59:59.5, did not convert at all. Split with floorDiv/floorMod so the remainder is never negative, and share the split between the two methods rather than repeating it once per supported precision. The set of supported precisions and both failure messages are unchanged. --- .../expression/ExpressionRexConverter.java | 100 +++++++++--------- .../substrait/isthmus/CalciteLiteralTest.java | 33 ++++++ 2 files changed, 81 insertions(+), 52 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java index ef7904a8e..46faa13cf 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java @@ -305,36 +305,10 @@ public RexNode visit(PrecisionTimeLiteral expr, Context context) throws RuntimeE * @return a TimeString representing the time value */ protected TimeString createTimeString(long value, int precision) { - switch (precision) { - case 0: - return TimeString.fromMillisOfDay((int) TimeUnit.SECONDS.toMillis(value)); - case 3: - { - long seconds = TimeUnit.MILLISECONDS.toSeconds(value); - int fracSecondsInNano = - (int) (TimeUnit.MILLISECONDS.toNanos(value) - TimeUnit.SECONDS.toNanos(seconds)); - return TimeString.fromMillisOfDay((int) TimeUnit.SECONDS.toMillis(seconds)) - .withNanos(fracSecondsInNano); - } - case 6: - { - long seconds = TimeUnit.MICROSECONDS.toSeconds(value); - int fracSecondsInNano = - (int) (TimeUnit.MICROSECONDS.toNanos(value) - TimeUnit.SECONDS.toNanos(seconds)); - return TimeString.fromMillisOfDay((int) TimeUnit.SECONDS.toMillis(seconds)) - .withNanos(fracSecondsInNano); - } - case 9: - { - long seconds = TimeUnit.NANOSECONDS.toSeconds(value); - int fracSecondsInNano = (int) (value - TimeUnit.SECONDS.toNanos(seconds)); - return TimeString.fromMillisOfDay((int) TimeUnit.SECONDS.toMillis(seconds)) - .withNanos(fracSecondsInNano); - } - default: - throw new IllegalArgumentException( - String.format("Cannot handle PrecisionTime with precision %d.", precision)); - } + long unitsPerSecond = unitsPerSecond(precision, "PrecisionTime"); + return TimeString.fromMillisOfDay( + (int) TimeUnit.SECONDS.toMillis(secondsOf(value, unitsPerSecond))) + .withNanos(nanosOf(value, unitsPerSecond)); } @Override @@ -384,38 +358,60 @@ public RexNode visit(PrecisionTimestampTZLiteral expr, Context context) throws R } private TimestampString getTimestampString(long value, int precision) { + long unitsPerSecond = unitsPerSecond(precision, "PrecisionTimestamp"); + return TimestampString.fromMillisSinceEpoch( + TimeUnit.SECONDS.toMillis(secondsOf(value, unitsPerSecond))) + .withNanos(nanosOf(value, unitsPerSecond)); + } + + /** + * Returns how many units of a temporal value at the given precision make up one second. + * + * @param precision the fractional-second precision + * @param typeName the Substrait type being converted, for the failure message + * @return the number of units per second + * @throws IllegalArgumentException if the precision is not one Calcite can be given + */ + private static long unitsPerSecond(int precision, String typeName) { switch (precision) { case 0: - return TimestampString.fromMillisSinceEpoch(TimeUnit.SECONDS.toMillis(value)); + return 1L; case 3: - { - long seconds = TimeUnit.MILLISECONDS.toSeconds(value); - int fracSecondsInNano = - (int) (TimeUnit.MILLISECONDS.toNanos(value) - TimeUnit.SECONDS.toNanos(seconds)); - return TimestampString.fromMillisSinceEpoch(TimeUnit.SECONDS.toMillis(seconds)) - .withNanos(fracSecondsInNano); - } + return 1_000L; case 6: - { - long seconds = TimeUnit.MICROSECONDS.toSeconds(value); - int fracSecondsInNano = - (int) (TimeUnit.MICROSECONDS.toNanos(value) - TimeUnit.SECONDS.toNanos(seconds)); - return TimestampString.fromMillisSinceEpoch(TimeUnit.SECONDS.toMillis(seconds)) - .withNanos(fracSecondsInNano); - } + return 1_000_000L; case 9: - { - long seconds = TimeUnit.NANOSECONDS.toSeconds(value); - int fracSecondsInNano = (int) (value - TimeUnit.SECONDS.toNanos(seconds)); - return TimestampString.fromMillisSinceEpoch(TimeUnit.SECONDS.toMillis(seconds)) - .withNanos(fracSecondsInNano); - } + return 1_000_000_000L; default: throw new IllegalArgumentException( - String.format("Cannot handle PrecisionTimestamp with precision %d.", precision)); + String.format("Cannot handle %s with precision %d.", typeName, precision)); } } + /** + * Returns the whole seconds in a temporal value, rounding towards negative infinity rather than + * towards zero. Before the epoch the value is negative, and both {@link TimeString#withNanos} and + * {@link TimestampString#withNanos} reject the negative remainder that truncation would leave. + * + * @param value the temporal value + * @param unitsPerSecond the number of units of that value per second + * @return the whole seconds + */ + private static long secondsOf(long value, long unitsPerSecond) { + return Math.floorDiv(value, unitsPerSecond); + } + + /** + * Returns the sub-second part of a temporal value in nanoseconds, always in [0, 1_000_000_000). + * + * @param value the temporal value + * @param unitsPerSecond the number of units of that value per second + * @return the sub-second part in nanoseconds + */ + private static int nanosOf(long value, long unitsPerSecond) { + return (int) (Math.floorMod(value, unitsPerSecond) * (1_000_000_000L / unitsPerSecond)); + } + @Override public RexNode visit(Expression.IntervalYearLiteral expr, Context context) throws RuntimeException { diff --git a/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java b/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java index bdd99bb75..523eed19d 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java @@ -141,6 +141,39 @@ void tTimeWithNanoSecond() { rex.makeTimeLiteral(new TimeString("14:22:47.123456"), 6)); } + @Test + void tPrecisionTimestampBeforeTheEpoch() { + // 1969-12-31 23:59:59.5 at each precision Calcite can be given. The value is negative and the + // sub-second part is not, so splitting it has to floor rather than truncate towards zero: + // TimestampString.withNanos rejects the negative remainder truncation leaves behind. + assertEquals(new TimestampString("1969-12-31 23:59:59"), timestampStringOf(-1L, 0)); + assertEquals(new TimestampString("1969-12-31 23:59:59.5"), timestampStringOf(-500L, 3)); + assertEquals(new TimestampString("1969-12-31 23:59:59.5"), timestampStringOf(-500_000L, 6)); + + // The epoch itself, and a value after it, are unchanged by the same split. + assertEquals(new TimestampString("1970-01-01 00:00:00"), timestampStringOf(0L, 3)); + assertEquals(new TimestampString("1970-01-01 00:00:01.5"), timestampStringOf(1_500L, 3)); + } + + @Test + void tPrecisionTimeKeepsItsSubSecondPart() { + // A time of day is never negative, so this only guards that the shared split did not change + // what it produced. + assertEquals( + new TimeString("14:22:47.5"), + ((RexLiteral) + ExpressionCreator.precisionTime(false, (14L * 3600 + 22 * 60 + 47) * 1000 + 500, 3) + .accept(expressionRexConverter, Context.newContext())) + .getValueAs(TimeString.class)); + } + + private TimestampString timestampStringOf(long value, int precision) { + RexNode converted = + ExpressionCreator.precisionTimestamp(false, value, precision) + .accept(expressionRexConverter, Context.newContext()); + return ((RexLiteral) converted).getValueAs(TimestampString.class); + } + @Test void tDate() { bitest( From 3ff105dc8ecb9d53c987d876ef35d832c37c940a Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 18 Aug 2026 22:48:40 +0300 Subject: [PATCH 2/2] fix(isthmus): reject an out-of-range precision_time by value Found reviewing the change above. A precision_time is a time of day, so a value below zero or a day or more is not one. Neither is reported as such today: a negative value reaches TimeString.fromMillisOfDay and comes back as a corrupt time string (Invalid time format: [00:00:0/]), and a value of a day or more is either Guava's "Hour out of range: [24]" from inside TimeString or, once the millisecond count overflows an int, a silently wrong time - 4346734 seconds converts to 14:22:46. A range check names the value instead, and makes the narrowing to int provably safe. --- .../expression/ExpressionRexConverter.java | 6 +++ .../substrait/isthmus/CalciteLiteralTest.java | 45 +++++++++++++++---- 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java index 46faa13cf..80a1768e1 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java @@ -306,6 +306,12 @@ public RexNode visit(PrecisionTimeLiteral expr, Context context) throws RuntimeE */ protected TimeString createTimeString(long value, int precision) { long unitsPerSecond = unitsPerSecond(precision, "PrecisionTime"); + if (value < 0 || value >= 86_400L * unitsPerSecond) { + // A precision_time is a time of day. Without this an out-of-range value reaches + // TimeString.fromMillisOfDay, which reports a corrupt time string rather than the value. + throw new IllegalArgumentException( + String.format("Cannot handle PrecisionTime with out-of-range value %d.", value)); + } return TimeString.fromMillisOfDay( (int) TimeUnit.SECONDS.toMillis(secondsOf(value, unitsPerSecond))) .withNanos(nanosOf(value, unitsPerSecond)); diff --git a/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java b/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java index 523eed19d..3443b00bd 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java @@ -2,6 +2,7 @@ import static io.substrait.isthmus.SubstraitTypeSystem.YEAR_MONTH_INTERVAL; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.ImmutableMap; @@ -143,10 +144,9 @@ void tTimeWithNanoSecond() { @Test void tPrecisionTimestampBeforeTheEpoch() { - // 1969-12-31 23:59:59.5 at each precision Calcite can be given. The value is negative and the - // sub-second part is not, so splitting it has to floor rather than truncate towards zero: - // TimestampString.withNanos rejects the negative remainder truncation leaves behind. - assertEquals(new TimestampString("1969-12-31 23:59:59"), timestampStringOf(-1L, 0)); + // 1969-12-31 23:59:59.5, where the value is negative and the sub-second part is not. Splitting + // it has to floor rather than truncate towards zero: TimestampString.withNanos rejects the + // negative remainder truncation leaves behind, so both of these threw before this change. assertEquals(new TimestampString("1969-12-31 23:59:59.5"), timestampStringOf(-500L, 3)); assertEquals(new TimestampString("1969-12-31 23:59:59.5"), timestampStringOf(-500_000L, 6)); @@ -160,11 +160,26 @@ void tPrecisionTimeKeepsItsSubSecondPart() { // A time of day is never negative, so this only guards that the shared split did not change // what it produced. assertEquals( - new TimeString("14:22:47.5"), - ((RexLiteral) - ExpressionCreator.precisionTime(false, (14L * 3600 + 22 * 60 + 47) * 1000 + 500, 3) - .accept(expressionRexConverter, Context.newContext())) - .getValueAs(TimeString.class)); + new TimeString("14:22:47.5"), timeStringOf((14L * 3600 + 22 * 60 + 47) * 1000 + 500, 3)); + } + + @Test + void tPrecisionTimeRejectsAnOutOfRangeValue() { + // A precision_time is a time of day, so a day or more is as far outside it as a negative value + // is. Left to Calcite that end reports "Hour out of range: [24]" from inside TimeString until + // the millisecond count overflows an int, past which the narrowing wraps and 4346734 seconds + // converts to 14:22:46 with nothing said. Rejected by value instead. + assertEquals( + "Cannot handle PrecisionTime with out-of-range value -500.", timeRejectionOf(-500L, 3)); + assertEquals( + "Cannot handle PrecisionTime with out-of-range value 4346734.", + timeRejectionOf(4_346_734L, 0)); + assertEquals( + "Cannot handle PrecisionTime with out-of-range value 86400.", timeRejectionOf(86_400L, 0)); + + // The last second that is still a time of day, and the first, are not. + assertEquals(new TimeString("23:59:59"), timeStringOf(86_399L, 0)); + assertEquals(new TimeString("00:00:00"), timeStringOf(0L, 0)); } private TimestampString timestampStringOf(long value, int precision) { @@ -174,6 +189,18 @@ private TimestampString timestampStringOf(long value, int precision) { return ((RexLiteral) converted).getValueAs(TimestampString.class); } + private TimeString timeStringOf(long value, int precision) { + RexNode converted = + ExpressionCreator.precisionTime(false, value, precision) + .accept(expressionRexConverter, Context.newContext()); + return ((RexLiteral) converted).getValueAs(TimeString.class); + } + + private String timeRejectionOf(long value, int precision) { + return assertThrows(IllegalArgumentException.class, () -> timeStringOf(value, precision)) + .getMessage(); + } + @Test void tDate() { bitest(