comphead commented on code in PR #7522:
URL: https://github.com/apache/datafusion-comet/pull/7522#discussion_r4485579229
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -729,8 +761,15 @@ fn parse_string_to_decimal(
// fit in an int (e.g. "1e-2147483647" to a negative target scale), so do
not round to zero.
// Spark raises a raw `ArithmeticException` there in every eval mode.
Spark 4.0.5, 4.1.4 and
// 4.2.1 skip `setScale` when the value is far below the target scale's
ulp (SPARK-60119) and
- // return 0 for that example; see
https://github.com/apache/datafusion-comet/issues/6966.
+ // return 0 for that example; see
https://github.com/apache/datafusion-comet/issues/6966 and
+ // https://github.com/apache/datafusion-comet/issues/7187. A difference
below the int range
+ // puts a value of at most 38 digits more than 2^31 places below the
target scale, so it
+ // rounds to 0 whatever its sign. A difference above the int range is a
value too large for
+ // any precision, which stays an error.
if i32::try_from(scale_adjustment).is_err() {
Review Comment:
A related gap that predates this PR. On the releases without SPARK-60119,
`Decimal.set` keeps the parsed `BigDecimal` and `changePrecision` calls
`setScale` for any scale change, and with JDK 17 `setScale` fails well inside
the `i32` range. `new BigDecimal("1e-600000000").setScale(2, HALF_UP)` and
`setScale(-2)` both throw `ArithmeticException: BigInteger would overflow
supported range`, while `1e-50000000` still gives `0.00` after about 12
seconds. `'1e-2147483647'` to scale -1 throws `Underflow` as well, although its
`scale_adjustment` here is -2147483648, which fits in an `i32`. So without
`tiny_to_zero` the native cast returns 0 for those strings where those Spark
releases fail, and that includes positive target scales such as
`DECIMAL(10,2)`, which a negative-scale fallback would not cover. The bound is
a JDK detail rather than an int limit, so this may be better as its own issue
than in this PR. I checked the JDK behavior with a small Java program and read
`Decimal.set` and
`changePrecision` at `v4.1.3`, but have not run the cast through Spark.
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -200,6 +200,7 @@ object CometCast
castBuilder.setIsSpark4Plus(isSpark40Plus)
castBuilder.setDecimalZeroAnyExponent(
SparkFixes.castsZeroWithAnyExponentToDecimal(SPARK_VERSION))
+
castBuilder.setDecimalTinyToZero(SparkFixes.castsTinyStringToDecimalZero(SPARK_VERSION))
Review Comment:
On #7187 @kazuyukitanimura suggested falling back first. Was that
considered? `canCastFromString` could return `Unsupported` for a `DecimalType`
target with a negative scale, as `canCastFromByte` through `canCastFromLong`
already do with `negativeScaleDecimalFromIntegralReason`. Spark's own `Cast`
would then run through the codegen dispatcher on every version. That also
matches the older releases, where the new user guide paragraph says Comet
cannot reproduce the raw `ArithmeticException`, and a vendor build that
backports SPARK-60119. It would not need the new proto field either.
It would also cover a case this PR leaves out. With
`allowNegativeScaleOfDecimal=true`, `fromStringANSI` skips the integral-digit
check, and on `v4.1.4` `changePrecision` now returns overflow for
`'1e2147483647'` to `DECIMAL(10,-2)`, so ANSI raises
`NUMERIC_VALUE_OUT_OF_RANGE`. The native cast takes the `scale_adjustment > 38`
branch and raises `CAST_INVALID_INPUT`. Negative scales need a legacy flag, so
I expect the cost of the fallback to be small. I have not run it.
--
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]
