Raviston6296 commented on PR #60056:
URL: https://github.com/apache/spark/pull/60056#issuecomment-6656108605
Thanks for the fix. I went through it and it looks correct to me:
- `bitLength() < 32` is the right guard for `intValue()`: `Int.MinValue` has
a bit length of 31, so it is accepted, and `-2^31 - 1` has 32, so it is
rejected. For INT, `intVal` is always inside `[Int.MinValue, Int.MaxValue]`, so
the 37-digit case in `DecimalSuite` is what pins the `bitLength` check.
- `toBigInteger` truncates toward zero for negative values too, matching the
non-ANSI path (`Decimal.toInt` -> `toLong`), and the new tests cover the
negative side (`-0.999999999999999999`, the minimum bounds).
- The only callers are the codegen path (`castDecimalToIntegralTypeCode` ->
`roundTo${type}`) and `DecimalExactNumeric` for the interpreted one, so nothing
else depended on the old double rounding.
One question about backports: the double-based check has been in
`roundToNumeric` since SPARK-40285 (3.4.0), and it is still there on branch-3.5
and every 4.x branch. ANSI mode has been the default since 4.0, so this silent
wrong result affects the default cast behavior on all released 4.x versions. Do
you plan to backport it? If so, the `serialVersionUID` change matters more in a
maintenance release. Each branch's `UdfSerialization` `Decimal` entry would
need its own audit. Alternatively, a backport could keep the `(Double => T)`
lambdas at the call sites, unused, so the class shape and its computed SUID
stay the same. I have not checked the second option; it is only an idea.
--
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]
