viirya commented on code in PR #59335:
URL: https://github.com/apache/spark/pull/59335#discussion_r4238665056
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -458,6 +458,19 @@ 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) {
Review Comment:
Thanks. This is the Int overflow in `numDigitsInIntegralPart`, which is
fixed separately in #59334 (SPARK-60118) together with non-legacy
`CastWithAnsiOffSuite`/`CastWithAnsiOnSuite` cases for `1e2147483647`,
`12e2147483647`, etc. This PR covers the remaining cases that reach
`changePrecision`.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -458,6 +458,19 @@ 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 takes time proportional to the scale change and fails once
the change
+ // exceeds the Int range, e.g. for 1e-2147483647. Skip it when the
result is known.
+ val numIntegralDigits = dv.precision.toLong - dv.scale
+ if (dv.signum != 0 && numIntegralDigits > precision.toLong - scale) {
+ return false
+ }
+ if (dv.signum == 0 || numIntegralDigits < -scale.toLong - 1) {
+ // |dv| is below 0.01 ulp of the new scale, so the result depends
only on the sign
+ // and the rounding mode. Round a value of the same sign and 0.01
ulp instead.
+ dv = BigDecimal(dv.signum, scale + 2)
+ }
+ }
dv = dv.setScale(scale, roundMode)
Review Comment:
Agreed that `set(decimal, precision, scale)` and `set(decimal)` have the
same issue. I'd like to keep this PR focused on `changePrecision`. The parser
path also needs a decision on how to report overflow there
(`NUMERIC_VALUE_OUT_OF_RANGE.WITHOUT_SUGGESTION` carries a `roundedValue`).
Filed SPARK-60133 for it.
##########
sql/api/src/main/scala/org/apache/spark/sql/errors/DataTypeErrors.scala:
##########
@@ -210,7 +210,9 @@ private[sql] object DataTypeErrors extends
DataTypeErrorsBase {
new SparkArithmeticException(
errorClass = "NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION",
messageParameters = Map(
- "value" -> value.toPlainString,
+ // A negative scale (legacy mode only) can make the plain string
arbitrarily long,
+ // e.g. 1E+2147483647.
+ "value" -> (if (value.scale < 0) value.toString else
value.toPlainString),
Review Comment:
Good point. It now uses `toString` only when the scale is below -1000, so
the message for ordinary values like `1E+2` and `1E+40` is unchanged.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -481,6 +481,17 @@ class CastWithAnsiOnSuite extends CastSuiteBase with
QueryErrorsBase {
castErrMsg("abcd", DecimalType(38, 1)))
}
+ test("SPARK-60119: cast string with a huge exponent to decimal with negative
scale allowed") {
+ withSQLConf(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key ->
"true") {
+ Seq("1e2147483647", "-1e2147483646", "12e2147483647",
"1e100000000").foreach { str =>
+ checkExceptionInExpression[ArithmeticException](
+ cast(str, DecimalType(10, 2)),
+ "cannot be represented as Decimal(10, 2)")
Review Comment:
Added a `checkError` with the expected `value` for `1e2147483647`
(`1E+2147483647`) and for `1e40` (plain notation).
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -492,4 +492,57 @@ 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.1 ulp of the target scale rounds to 0 or +/-1 ulp
depending only on its
Review Comment:
Done.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -108,15 +108,8 @@ private[sql] object QueryExecutionErrors extends
QueryErrorsBase with ExecutionE
decimalPrecision: Int,
decimalScale: Int,
context: QueryContext = null): ArithmeticException = {
- new SparkArithmeticException(
- errorClass = "NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION",
- messageParameters = Map(
- "value" -> value.toPlainString,
- "precision" -> decimalPrecision.toString,
- "scale" -> decimalScale.toString,
- "config" -> toSQLConf(SQLConf.ANSI_ENABLED.key)),
- context = getQueryContext(context),
- summary = getSummary(context))
+ DataTypeErrors.cannotChangeDecimalPrecisionError(
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]
