dongjoon-hyun commented on code in PR #59335:
URL: https://github.com/apache/spark/pull/59335#discussion_r4247351758
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -481,6 +481,36 @@ 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)")
+ }
+ checkEvaluation(cast("0e2147483647", DecimalType(10, 2)),
Decimal("0.00"))
+
+ // The value is shown in plain notation unless its scale is hugely
negative.
+ Seq("1e2147483647" -> "1E+2147483647", "1e40" -> ("1" + "0" *
40)).foreach {
+ case (str, value) =>
+ if (!isTryCast) {
+ checkError(
+ exception = intercept[SparkArithmeticException](
+ cast(str, DecimalType(10, 2)).eval()),
Review Comment:
nit. This `checkError` runs only the interpreted path (`.eval()`). The
codegen path goes through `CastUtils.changePrecisionExact`, whose error helper
this PR switches from `QueryExecutionErrors` to `DataTypeErrors`. The loop
above checks that path only by a message substring, so the `value` parameter is
not asserted there. `checkErrorInExpression[SparkArithmeticException](cast(str,
DecimalType(10, 2)), "NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION", Map(...))`
checks the parameters in the interpreted, mutable-projection, and
unsafe-projection modes. `TryCastSuite` already overrides it to expect `null`,
so the `isTryCast` branch would no longer be needed. It does not check
`queryContext`, but that is the same empty context in every mode here.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -458,6 +458,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) {
Review Comment:
This shortcut fixes `changePrecision`, but `RoundBase` still rescales
without a bound. For a negative `_scale`, `nullSafeEval` calls
`decimal.toBigDecimal.setScale(_scale, mode)` directly
(`mathExpressions.scala:1782`). The byte/short/int/long branches
(`mathExpressions.scala:1786-1807`) and the generated code
(`mathExpressions.scala:1856-1857`) do the same. On the JDK:
```
jshell> new BigDecimal("1.5").setScale(-2147483646, RoundingMode.HALF_UP)
java.lang.ArithmeticException: BigInteger would overflow supported range
jshell> new BigDecimal("1.5").setScale(-2147483647, RoundingMode.HALF_UP)
java.lang.ArithmeticException: Underflow
```
So `SELECT round(1.5BD, -2147483646)` fails with a raw `ArithmeticException`
instead of returning `0`. Unlike the cast cases, this does not need
`spark.sql.legacy.allowNegativeScaleOfDecimal`, because `round`/`bround` accept
any foldable Int scale.
This is pre-existing, so it does not need to block this PR. Since
SPARK-60133 already covers `Decimal.set(decimal, precision, scale)`, could we
add `round`/`bround` to its scope? A shared helper in `Decimal` built from the
logic here could then serve all of these callers.
##########
sql/api/src/main/scala/org/apache/spark/sql/errors/DataTypeErrors.scala:
##########
@@ -207,10 +212,14 @@ private[sql] object DataTypeErrors extends
DataTypeErrorsBase {
decimalPrecision: Int,
decimalScale: Int,
context: QueryContext): ArithmeticException = {
+ // A huge negative scale (legacy mode only) makes the plain string too
long to build,
+ // e.g. for 1E+2147483647.
+ val valueString =
+ if (value.scale < -MAX_PLAIN_STRING_TRAILING_ZEROS) value.toString else
value.toPlainString
Review Comment:
This bounds the plain string only in the `NUMERIC_VALUE_OUT_OF_RANGE`
message. Other code still calls `Decimal.toPlainString` on the same values:
- `Cast` to string uses plain notation under ANSI (`useDecimalPlainString =
ansiEnabled`, `Cast.scala:829`, used at `ToStringBase.scala:200` and
`ToStringBase.scala:353`).
- `ToPrettyString`, which `Dataset.show` uses, always does
(`ToPrettyString.scala:50`).
`DecimalType` has no lower bound on a negative scale. With
`spark.sql.legacy.allowNegativeScaleOfDecimal=true` and the default ANSI mode,
both of these build the full plain string:
```scala
lit("1e2147483647").cast(DecimalType(1, -2147483647)).cast("string")
spark.range(1).select(lit(new java.math.BigDecimal("1e2147483647"))).show()
```
The inner cast succeeds because the precision and scale already match, so
`changePrecision` takes its fast path. On the JDK, `new
BigDecimal("1e2147483647").toPlainString()` throws `OutOfMemoryError: too large
to fit in a String`.
This is pre-existing and legacy-only, so it does not need to block this PR.
But it is the same problem this guard solves. Could we track it in a JIRA, or
move the threshold into a `Decimal`-level helper that these callers can share?
--
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]
