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..80a1768e1 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,16 @@ 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"); + 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)); } @Override @@ -384,38 +364,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..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; @@ -141,6 +142,65 @@ void tTimeWithNanoSecond() { rex.makeTimeLiteral(new TimeString("14:22:47.123456"), 6)); } + @Test + void tPrecisionTimestampBeforeTheEpoch() { + // 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)); + + // 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"), 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) { + RexNode converted = + ExpressionCreator.precisionTimestamp(false, value, precision) + .accept(expressionRexConverter, Context.newContext()); + 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(