spark-test commented on code in PR #60188:
URL: https://github.com/apache/spark/pull/60188#discussion_r4500500771
##########
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:
**Non-blocking (P2):** With
`spark.sql.legacy.allowNegativeScaleOfDecimal=true`, a negative operand scale
feeds `digits` directly, and this context has no upper limit. For `1E+100000BD
% 1BD`, `digits` is 100,001, although the integral quotient has a single
significant digit. The JDK computes all of those quotient digits and then
strips the trailing zeros one at a time.
I ran the same `BigDecimal` calls on JDK 21.0.8, comparing the base branch's
`MATH_CONTEXT` with this helper. Both return the same values where both succeed:
- `1E+10000BD % 1BD`: 57 ms, against under 1 ms on the base branch
- `1E+100000BD % 1BD`: 3.6 s
- `1E+1000000BD % 1BD`: no answer within 150 s, so a query that used to
finish can hang during constant folding or on every row
- `1E+999999999BD % 1BD`: now raises `ArithmeticException: BigInteger would
overflow supported range` instead of returning 0
- `1E+2147483636BD % 3E-38BD`: the bound exceeds `Int.MaxValue`, so
`digits.toInt` wraps negative and `new MathContext` raises
`IllegalArgumentException: Digits < 0`, where the base branch raised
`ArithmeticException`
So under this mode, some calls that did not raise before now fail, and
others become far slower. This is the kind of extreme negative-scale cost and
`BigInteger` overflow that SPARK-60119 recently removed from `changePrecision`.
With scales 0 to 38 the bound stays at or below 76, so this part is specific to
the legacy mode.
See **Shared repair plan 1** in the review body.
##########
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:
**Nit (P3):** This covers the widened-context branch at one width only: a
41-digit quotient. The randomized SPARK-45786 test never reaches the branch,
because `makeNum` always yields |divisor| >= 0.1, which keeps the bound at 39
or below. Two plausible regressions would leave the suite green while supported
inputs raise `Division impossible` again:
- An off-by-one threshold such as `digits <= 40` breaks 38 nines `% 0.07`,
which has a 40-digit quotient and remainder 0.02.
- A fixed 41-digit context breaks 38 nines `% 7E-38` as `DECIMAL(38, 38)`,
which has a 76-digit quotient and remainder 2E-38. That is the widest case for
precision-38 operands with non-negative scales.
Both inputs raise on the base branch and return those remainders with this
change (I checked both on JDK 21). Asserting them next to the 41-digit case
would pin both edges.
**Verification:**
- **Behavior:** Remainder and Pmod return 0.02 for 38 nines by 0.07 and
2E-38 for 38 nines by 7E-38 in LEGACY and ANSI modes on interpreted and
code-generated evaluation. IntegralDivide returns NULL in LEGACY and raises
NUMERIC_VALUE_OUT_OF_RANGE in ANSI for both inputs.
- **Regression:** Keeping the 39-digit context when the bound is 40 makes
the 40-digit case fail with the raw Division impossible exception. Using a
fixed 41-digit context makes the 76-digit case fail. Both cases pass with the
PR's context selection, and with the fix that keeps the 39-digit context until
it raises.
--
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]
