spark-test commented on code in PR #59889:
URL: https://github.com/apache/spark/pull/59889#discussion_r4462394271
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -556,9 +556,30 @@ final class Decimal extends Ordered[Decimal] with
Serializable {
if (that.isZero) null
else Decimal(toJavaBigDecimal.divide(that.toJavaBigDecimal, scale,
RoundingMode.HALF_UP))
- def %(that: Decimal): Decimal =
- if (that.isZero) null
- else Decimal(toJavaBigDecimal.remainder(that.toJavaBigDecimal,
MATH_CONTEXT))
+ def %(that: Decimal): Decimal = {
+ if (that.isZero) {
+ null
+ } else {
+ val dividend = toJavaBigDecimal
+ val divisor = that.toJavaBigDecimal
+ // Try the context first for speed and for huge trailing-zero quotients
that exact division
+ // cannot materialize. On failure, retry without its quotient precision
limit: a quotient
+ // wider than the context may still have a representable remainder.
+ val remainder =
+ try {
+ dividend.remainder(divisor, MATH_CONTEXT)
+ } catch {
+ case error: ArithmeticException =>
+ // Exact division materializes the integral quotient. Two maximum
SQL decimal
+ // precisions cover in-range SQL operands; wider estimates (legacy
negative scales or
+ // values beyond SQL bounds) keep the original failure to avoid a
huge allocation.
+ val quotientDigits = dividend.precision().toLong -
dividend.scale() + divisor.scale()
+ if (quotientDigits > 2 * DecimalType.MAX_PRECISION) throw error
Review Comment:
**Non-blocking (P2):** This cutoff leaves part of the failure this PR fixes
in place. The estimate follows SQL precision rather than the work the remainder
needs, so with `spark.sql.legacy.allowNegativeScaleOfDecimal=true`:
- `SELECT 1E75BD % 3BD` has an estimate of 76 and now returns 1 through the
exact retry.
- `SELECT 1E76BD % 3BD` has an estimate of 77 and still throws the raw
`java.lang.ArithmeticException: Division impossible`. Its remainder 1 fits the
`DECIMAL(1,0)` result type, and its exact quotient has 76 digits, the same size
the retry already builds for `99999999999999999999999999999999999999 % 7E-38`.
`pmod` and numeric `try_mod` split the same way. `try_mod` evaluates
`Remainder` in TRY mode without catching the exception, so neither it nor
non-ANSI mode turns the failure into NULL. Direct `Decimal` values beyond SQL
bounds are affected too, for example `Decimal(1) % Decimal("3E-100")`. In-range
SQL operands never exceed 76, so default-configuration queries are fully fixed.
The affected inputs are those whose estimate exceeds 76 while the 39-digit
context division fails.
The allocation risk the cap guards against comes from materializing the
quotient, which computing the remainder does not require. Only huge scale gaps,
such as the -1000000000 dividend scale in the new bound test, make exact
division expensive.
**Recommended change:** When the 39-digit context remainder fails and the
quotient-size estimate exceeds 76 digits, compute the remainder with bounded
work instead of rethrowing: use modular arithmetic on the unscaled values when
the divisor's scale is at least the dividend's, and the existing exact retry
otherwise. Representable remainders for legacy negative scales and out-of-range
Decimal values are then returned. Update the bound comment, and change the
Decimal remainder-bound tests that now assert the rethrow so they assert the
computed remainders. Add legacy negative-scale cases for `%` and `pmod` to the
arithmetic expression tests, and for numeric `try_mod` to the TRY-mode
remainder test.
**Why this works:** Let u1 and u2 be the dividend's and divisor's unscaled
values and s1 and s2 their scales. When the divisor scale is at least the
dividend scale, the truncated remainder is sign(dividend) * ((|u1| mod |u2|) *
(10^(s2 - s1) mod |u2|) mod |u2|), at the divisor's scale. BigInteger.modPow
computes the power of ten modulo |u2| in steps logarithmic in the scale gap, so
no wide quotient or large power of ten is ever materialized. When the dividend
scale is larger, a failing context division implies the dividend has more
digits than the scale gap, so the existing exact retry stays bounded by the
operand sizes and can be used for any estimate. The context call stays first,
and the exact retry for estimates up to 76 stays as it is, so every result the
base branch or the current fallback produces keeps its value and scale.
**Scope:** Extend the remainder fallback in Decimal.% so inputs above the
current quotient-size cap are computed with bounded work rather than rethrown,
and update the remainder tests to match.
**Compatibility:** The 39-digit context remainder stays the first attempt,
and the exact retry still handles estimates up to 76 digits, so every value and
scale produced today, on the base branch or by the current fallback, is
unchanged. Zero divisors still return null.
**Risks:** Results on the new modular branch take the divisor's scale, the
larger operand scale. This matches the SQL remainder result scale. Exact
BigDecimal division can return a smaller scale when the quotient has trailing
zeros, so direct Decimal callers may get the divisor's scale where exact
division would return a smaller one, on inputs that throw today. The new
arithmetic must keep the dividend's sign and ignore the divisor's sign. A sign
mistake would silently return wrong remainders, so every operand-sign
combination needs coverage.
**Constraints:** Keep the 39-digit context call first and keep the exact
retry for estimates up to 76, so existing results keep their values and scales.
Compute the power of ten modulo the divisor's unscaled value (for example with
BigInteger.modPow), so the work grows with the logarithm of the scale gap and
never with the gap itself.
**Exclusions:**
- Integral division (Decimal.quot and SQL div) still throws the raw
exception for integral quotients wider than 39 digits. That is a separate
existing error-classification issue for a quotient that cannot fit its result
type, and it is left unchanged.
**Success:** With spark.sql.legacy.allowNegativeScaleOfDecimal enabled,
1E76BD % 3BD returns 1, 1E100BD % 7BD returns 4, pmod(-1E76BD, 3BD) returns 2,
and try_mod(1E76BD, 3BD) returns 1, in both interpreted and generated
evaluation. Decimal(1L, 1, -76) % Decimal(3L), Decimal(1L, 1, -1000000000) %
Decimal(3L), and Decimal(1L, 1, Int.MinValue) % Decimal(3L) return 1 without
materializing the quotient. Decimal(1) % Decimal("3E-100") returns 1E-100, and
Decimal(1) % Decimal("3E-100000000") returns 1E-100000000 promptly. A direct
dividend wider than 76 digits returns its exact remainder whether its scale is
at most or above the divisor's: 10^77 at scale 0 % 3 and 10^89 at scale 10 (100
digits) % 3 both return 1. Every input that the 39-digit context or the current
exact retry already handles keeps its value and scale, and a zero divisor still
returns null.
**Verification:**
- **Behavior:** Remainders whose quotient-size estimate exceeds the current
76-digit cap are returned exactly, with the dividend's sign, for `%`, `pmod`,
and numeric `try_mod` under the legacy negative-scale config, through
interpreted and generated evaluation. Cases include 1E76BD % 3BD = 1, 1E100BD %
7BD = 4, and pmod(-1E76BD, 3BD) = 2.
- **Regression:** Every input whose estimate exceeds 76 and whose context
division fails returns its exact remainder, with work bounded by the operand
sizes and the logarithm of the scale gap, on both fallback branches and in all
four operand-sign combinations. Dividend scales -76, -1000000000, and
Int.MinValue with divisor 3 return 1. Dividend 1 with divisors 3E-100 and
3E-100000000 returns 1E-100 and 1E-100000000. Direct dividends wider than 76
digits return 1 for 10^77 at scale 0 % 3 and for 10^89 at scale 10 % 3. These
replace the current assertions that such inputs rethrow the context failure.
- **Compatibility:** Inputs that the 39-digit context already handles, and
in-range inputs that the exact retry already fixes, keep identical values and
scales in all four operand-sign combinations. A zero divisor still returns null.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -245,6 +247,138 @@ class DecimalSuite extends SparkFunSuite with
PrivateMethodTester with SQLHelper
assert(Decimal(100) % Decimal(0) === null)
}
+ Seq(
+ ("10000000000000000000000000000000000000", "0.0007", "0.0005"),
+ ("99999999999999999999999999999999999999", "0.0007", "0.0004"),
+ ("99999999999999999999999999999999999999", "7E-38", "2E-38"),
+ ("99999999999999999999999999999999999999", "1E-38", "0")
+ ).foreach { case (dividend, divisor, expected) =>
+ test(s"decimal remainder with a large integral quotient: $dividend %
$divisor") {
+ val left = Decimal(dividend)
+ val right = Decimal(divisor)
+ val remainder = Decimal(expected)
+ assert(left % right == remainder)
+ assert(left % -right == remainder)
+ assert(-left % right == -remainder)
+ assert(-left % -right == -remainder)
+ assert((left % right).scale == remainder.scale)
+ assert(left % Decimal(0) == null)
+ }
+ }
+
+ Seq(
+ (38, "99999999999999999999999999999999999999", "7", "1", 38),
+ (39, "99999999999999999999999999999999999999", "0.7", "0.3", 39),
+ (40, "99999999999999999999999999999999999999", "0.07", "0.02", 40),
+ (40, "10000000000000000000000000000000000000", "0.07", "0.06", 39)
+ ).foreach { case (bound, dividend, divisor, expected, quotientDigits) =>
+ test(s"decimal remainder with quotient bound $bound and $quotientDigits
actual digits") {
+ val left = Decimal(dividend)
+ val right = Decimal(divisor)
+ val quotient =
left.toJavaBigDecimal.divideToIntegralValue(right.toJavaBigDecimal)
+ assert(quotient.toBigInteger.toString.length == quotientDigits)
+ for (leftSign <- Seq(1, -1); rightSign <- Seq(1, -1)) {
+ val signedLeft = if (leftSign < 0) -left else left
+ val signedRight = if (rightSign < 0) -right else right
+ val remainder = if (leftSign < 0) -Decimal(expected) else
Decimal(expected)
+ assert((signedLeft % signedRight).toJavaBigDecimal ==
remainder.toJavaBigDecimal)
+ }
+ }
+ }
+
+ test("decimal remainder preserves zero quotients, trailing zeros and compact
operands") {
+ Seq(
+ (Decimal("0.00"), Decimal("0.7"), Decimal("0.00")),
+ (Decimal("1.00"), Decimal("7.0"), Decimal("1.00")),
+ (Decimal(120000L, 6, 2), Decimal(70L, 2, 1), Decimal("3.00")),
+ // The 40-digit quotient is representable by the old context because it
has trailing zeros.
Review Comment:
**Nit (P3):** `%` still tries this same 39-digit `MATH_CONTEXT` first, and
there is no other context, so after merge "old" refers to nothing. It also
hides what the case shows: 1E+39 has a single significant digit, so this case
stays on the context path and never reaches the exact retry.
```suggestion
// The 39-digit context represents this 40-digit quotient because it
has trailing zeros.
```
**Verification:**
- **Inspection:** The comment above the trailing-zero remainder case names
the current 39-digit context rather than an old one, and no assertion in that
test changes.
--
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]
