dongjoon-hyun commented on code in PR #60188:
URL: https://github.com/apache/spark/pull/60188#discussion_r4500602387
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -558,14 +558,34 @@ final class Decimal extends Ordered[Decimal] with
Serializable {
def %(that: Decimal): Decimal =
if (that.isZero) null
- else Decimal(toJavaBigDecimal.remainder(that.toJavaBigDecimal,
MATH_CONTEXT))
+ else {
+ val (a, b) = (toJavaBigDecimal, that.toJavaBigDecimal)
+ Decimal(a.remainder(b, integralQuotientContext(a, b)))
+ }
def quot(that: Decimal): Decimal =
if (that.isZero) null
- else Decimal(toJavaBigDecimal.divideToIntegralValue(that.toJavaBigDecimal,
MATH_CONTEXT))
+ else {
+ val (a, b) = (toJavaBigDecimal, that.toJavaBigDecimal)
+ Decimal(a.divideToIntegralValue(b, integralQuotientContext(a, b)))
+ }
def remainder(that: Decimal): Decimal = this % that
+ // `BigDecimal.remainder` and `divideToIntegralValue` raise "Division
impossible" when the
+ // integral part of the quotient has more digits than the `MathContext`
holds, e.g. 41 digits
+ // for DECIMAL(38, 0) and DECIMAL(3, 3). Returns `MATH_CONTEXT`, or a larger
context that holds
+ // the integral part of `a / b`: |a| < 10^(a.precision - a.scale) and
+ // |b| >= 10^(b.precision - b.scale - 1).
+ // This is a private method of the class, not of `object Decimal`: the class
calling a private
+ // member of the object makes it public in `Decimal$` and changes the
computed
+ // serialVersionUID of `Decimal$`, which
`UdfSerialization.auditedTransitions` pins.
+ private def integralQuotientContext(a: JavaBigDecimal, b: JavaBigDecimal):
MathContext = {
+ val digits = (a.precision.toLong - a.scale) - (b.precision.toLong -
b.scale) + 1
+ if (digits <= MATH_CONTEXT.getPrecision) MATH_CONTEXT
+ else new MathContext(digits.toInt, RoundingMode.DOWN)
Review Comment:
Thank you. I reproduced all of them, and also the slowdown of exact
quotients in the review body. Now, `%` and `quot` try `MATH_CONTEXT` first and
use a wider context only when it raises. The bound is also checked against the
`Int` range before the wider context is built. I added a `DecimalSuite` test
with `1E+999999999 % 1` and `1E+2147483636 % 3E-38` under
`spark.sql.legacy.allowNegativeScaleOfDecimal`.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala:
##########
@@ -776,6 +776,18 @@ class ArithmeticExpressionSuite extends SparkFunSuite with
ExpressionEvalHelper
}
}
+ test("SPARK-61050: Remainder, Pmod and IntegralDivide of decimals with a
41-digit quotient") {
Review Comment:
Thank you. The test now uses the 40- and 76-digit quotients, `% 0.07` and `%
7E-38`, instead of the 41-digit one.
--
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]
