viirya commented on code in PR #59335:
URL: https://github.com/apache/spark/pull/59335#discussion_r4262665765
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -492,4 +492,77 @@ class DecimalSuite extends SparkFunSuite with
PrivateMethodTester with SQLHelper
s"unscaled: $unscaled, scaleFrom: $scaleFrom, scaleTo: $scaleTo, mode:
$roundMode")
}
}
+
+ test("SPARK-60119: changePrecision with a source scale far from the target
scale") {
+ // A value below 0.01 ulp of the target scale rounds to 0 or +/-1 ulp
depending only on its
+ // sign and the rounding mode, so it must round like a small value of the
same sign.
+ Seq("1e-2147483647", "-1e-2147483647", "9.99e-100000000", "-1e-100000000",
+ "0e-2147483647").foreach { str =>
+ val bd = new java.math.BigDecimal(str)
+ allSupportedRoundModes.foreach { mode =>
+ val expected = BigDecimal(bd.signum, 50).setScale(2, mode)
+ val d = Decimal(bd)
+ assert(d.changePrecision(10, 2, mode), s"$str, $mode")
+ assert(d.toString === expected.toString, s"$str, $mode")
+ }
+ }
+
+ withSQLConf(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key ->
"true") {
+ Seq("1e2147483647", "-1e2147483646", "12e2147483647",
"1e100000000").foreach { str =>
+ allSupportedRoundModes.foreach { mode =>
+ val d = Decimal(new java.math.BigDecimal(str))
+ assert(d.toPrecision(10, 2, mode) === null, s"$str, $mode")
+ }
+ }
+ val zero = Decimal(new java.math.BigDecimal("0e2147483647"))
+ assert(zero.changePrecision(10, 2))
+ assert(zero.toString === "0.00")
+
+ // A compact (long) input takes the Long branch first, where the scale
change can exceed
+ // the Int range, and may switch to the BigDecimal branch. It must round
like the same
+ // value in BigDecimal form.
+ Seq(
+ (1L, 1, -40, 10, 2),
Review Comment:
Good point. Those cases now assert their expected results directly (`null`,
`"0.00"`), and I added `(1L, 1, Int.MinValue, 38, 20)` for the nonzero scale-up
case. Only the `100L`/`-100L` cases are still compared with the BigDecimal form.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -458,6 +459,20 @@ final class Decimal extends Ordered[Decimal] with
Serializable {
if (dv.ne(null)) {
// We get here if either we started with a BigDecimal, or we switched to
one because we would
// have overflowed our Long; in either case we must rescale dv to the
new scale.
+ if (math.abs(dv.scale.toLong - scale) > DecimalType.MAX_PRECISION) {
+ // setScale's cost grows with the scale change, and it throws once
10^change exceeds
+ // BigInteger's range (e.g. for 1e-2147483647) or the change exceeds
the Int range.
+ // Skip it when the result is known.
+ val numIntegralDigits = dv.precision.toLong - dv.scale
Review Comment:
Agreed. Since `numDigitsInIntegralPart` returns Int until #59334 merges,
calling it here now would bring the overflow back. Whichever of the two PRs
merges second will switch this to `numDigitsInIntegralPart(dv.underlying)`.
##########
sql/api/src/main/scala/org/apache/spark/sql/errors/DataTypeErrors.scala:
##########
@@ -31,6 +31,11 @@ import org.apache.spark.unsafe.types.UTF8String
* into [[CompilationErrors]].
*/
private[sql] object DataTypeErrors extends DataTypeErrorsBase {
+ // A decimal whose scale is below -MAX_PLAIN_STRING_TRAILING_ZEROS (that is,
with more than
+ // this many zeros implied by a negative scale) is rendered in scientific
notation in error
+ // messages instead of plain notation.
+ private val MAX_PLAIN_STRING_TRAILING_ZEROS = 1000
Review Comment:
Done.
--
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]
