viirya commented on code in PR #59335:
URL: https://github.com/apache/spark/pull/59335#discussion_r4251323795
##########
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:
Thanks. #58939 (SPARK-59686) is already working on this path: it clamps
`RoundBase._scale`, which every branch reads, including the decimal one and the
generated code. I'd rather leave it there than fold it into SPARK-60133. If
that PR stalls, I'll pick it up in SPARK-60133.
##########
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:
Good catch. Filed SPARK-60200 to bound the plain string for decimals with a
huge negative scale in cast-to-string and `ToPrettyString`, possibly with a
`Decimal`-level helper shared with this error message.
##########
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:
Done, with one change: `checkErrorInExpression` doesn't pass `queryContext`
to `checkError`, so it expects no context while this error carries an empty
one, and the check fails. The test now runs `checkError` over the interpreted,
mutable-projection and unsafe-projection paths itself.
--
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]
