cloud-fan commented on code in PR #56313:
URL: https://github.com/apache/spark/pull/56313#discussion_r3353064301
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -510,7 +510,7 @@ object DateTimeUtils extends SparkDateTimeUtils {
val offsetMicros = originalOffsetSec * MICROS_PER_SECOND
try {
val local = Math.addExact(micros, offsetMicros)
- val truncatedLocal = local - Math.floorMod(local, unitMicros)
+ val truncatedLocal = Math.subtractExact(local, Math.floorMod(local,
unitMicros))
Review Comment:
Nicely targeted fix. Note the same `value - Math.floorMod(value, unit)`
shape lives one match away in the sibling branches: `TRUNC_TO_MILLISECOND`
(line 540) and `TRUNC_TO_SECOND` (line 542) still use a plain `-`, so a
timestamp at `Long.MinValue` micros silently wraps to a positive value there
(e.g. `MIN - floorMod(MIN, 1000) = 9223372036854775616`) instead of throwing
like the fast path now does. Those branches have no offset arithmetic and no
slow-path fallback, so applying `Math.subtractExact` directly would make them
throw `ArithmeticException` consistently. Worth folding into this PR (with
MILLISECOND/SECOND added to the new test), or noting they're intentionally out
of scope. Pre-existing, so non-blocking.
--
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]
