MaxGekk commented on code in PR #57044:
URL: https://github.com/apache/spark/pull/57044#discussion_r3577457227
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1232,22 +1232,21 @@ object DateTimeUtils extends SparkDateTimeUtils {
* @param intervalEndField The rightmost field which the interval comprises
of.
* Valid values: 0 (DAY), 1 (HOUR), 2 (MINUTE), 3
(SECOND).
* @param targetPrecision The number of digits of the fraction part of the
resulting time.
- * @return A time value in nanoseconds or throw an arithmetic overflow
- * if the result out of valid time range [00:00, 24:00).
+ * @return A time value in nanoseconds, wrapped modulo 24 hours per ANSI SQL
semantics
+ * (TIME arithmetic that crosses midnight wraps around).
*/
def timeAddInterval(
time: Long,
timePrecision: Int,
interval: Long,
intervalEndField: Byte,
targetPrecision: Int): Long = {
- val result = MathUtils.addExact(time, MathUtils.multiplyExact(interval,
NANOS_PER_MICROS))
- if (0 <= result && result < NANOS_PER_DAY) {
- truncateTimeToPrecision(result, targetPrecision)
- } else {
- throw QueryExecutionErrors.timeAddIntervalOverflowError(
- time, timePrecision, interval, intervalEndField)
- }
+ // ANSI SQL modulo-24 semantics: TIME + INTERVAL wraps around midnight.
+ // Use floorMod to keep the result in [0, NANOS_PER_DAY) for both positive
+ // and negative overflow (e.g., 23:00 + 2h = 01:00, 01:00 - 3h = 22:00).
+ val intervalNanos = interval * NANOS_PER_MICROS
+ val result = Math.floorMod(time + intervalNanos, NANOS_PER_DAY)
Review Comment:
The old code used `MathUtils.multiplyExact(interval, NANOS_PER_MICROS)` /
`addExact(time, _)` — overflow-checked. These plain `*` and `+` are not:
`interval` is a `DayTimeIntervalType` value in microseconds (`Long`, up to
`Long.MaxValue`), so `interval * NANOS_PER_MICROS` overflows once the interval
exceeds ~9.22e15 μs. That input is reachable — the pre-PR build throws
`ARITHMETIC_OVERFLOW` for `TIME '12:00:00' + INTERVAL '106751991' DAY`, so the
large value flows into the multiply intact. On overflow the multiply wraps
silently and `floorMod` of the corrupted product returns a **wrong time** (I
measured `43964145223384` ns vs the correct `42437854775000` ns); with
`timeAddIntervalOverflowError` removed there's no error path left, so it's
silently wrong.
Reducing the interval modulo a day *in microseconds first* keeps every
operation in range (`reducedMicros * NANOS_PER_MICROS < NANOS_PER_DAY` and
`time < NANOS_PER_DAY`, so neither `*` nor `+` can overflow):
```suggestion
// ANSI SQL modulo-24 semantics: TIME + INTERVAL wraps around midnight.
// Reduce the interval modulo a day in microseconds first so neither the
// nanosecond multiply nor the add can overflow Long for large intervals
// (e.g., 23:00 + 2h = 01:00, 01:00 - 3h = 22:00).
val reducedMicros = Math.floorMod(interval, MICROS_PER_DAY)
val result = Math.floorMod(time + reducedMicros * NANOS_PER_MICROS,
NANOS_PER_DAY)
```
I verified this matches the correct wrapped value for the small cases, the
overflowing case, and both `Long.MinValue`/`Long.MaxValue` interval edges.
##########
sql/core/src/test/resources/sql-tests/inputs/time.sql:
##########
@@ -206,6 +206,13 @@ SELECT '00:00:00.0001' :: TIME(4) - INTERVAL '0
00:00:00.0001' DAY TO SECOND;
SELECT '08:30' :: TIME(0) - INTERVAL '6' HOUR;
SELECT '10:00:01' :: TIME(1) - INTERVAL '1' MONTH;
+-- SPARK-57853: TIME +/- INTERVAL modulo-24 wrap semantics
+SELECT TIME'23:00' + INTERVAL '2' HOUR;
+SELECT TIME'01:00' - INTERVAL '3' HOUR;
+SELECT TIME'00:00' - INTERVAL '0.000001' SECOND;
+SELECT TIME'12:00' + INTERVAL '24' HOUR;
+SELECT TIME'06:30' + INTERVAL '48' HOUR;
Review Comment:
These wrap cases top out at `INTERVAL '48' HOUR`, far below the overflow
threshold (~9.22e15 μs), so the large-interval bug above ships with green CI.
Add a large-day-interval case that pins the correct wrapped value — e.g.:
```sql
SELECT TIME'12:00:00' + INTERVAL '106751991' DAY;
```
(with the fix, `106751991 DAY mod 24h = 0`, so this should return
`12:00:00`). That both documents the wrap-at-scale behavior and guards against
the overflow regression silently returning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
