spark-test commented on code in PR #59889:
URL: https://github.com/apache/spark/pull/59889#discussion_r4406720191
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -245,6 +247,141 @@ 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.
+ (Decimal("10000000000000000000000000000000000000"), Decimal("0.01"),
Decimal("0"))
+ ).foreach { case (left, right, expected) =>
+ assert((left % right).toJavaBigDecimal == expected.toJavaBigDecimal)
+ assert((-left % right).toJavaBigDecimal == (-expected).toJavaBigDecimal)
+ assert((left % -right).toJavaBigDecimal == expected.toJavaBigDecimal)
+ assert(left % Decimal(0) == null)
+ }
+ }
+
+ test("decimal remainder preserves negative and extreme scales") {
+ withSQLConf(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key ->
"true") {
+ Seq(
+ (Decimal("123E+4"), Decimal("7E+2"), Decimal("1E+2")),
+ (Decimal("9E+39"), Decimal("7"), Decimal("5")),
+ (Decimal(3L, 1, Int.MinValue), Decimal(2L, 1, Int.MinValue),
+ Decimal(1L, 1, Int.MinValue)),
+ (Decimal(3L, 1, Int.MaxValue), Decimal(2L, 1, Int.MaxValue),
+ Decimal(1L, 1, Int.MaxValue)),
+ (Decimal(3L, 1, Int.MinValue), Decimal(2L, 1, Int.MinValue + 1),
+ Decimal(0L, 1, Int.MinValue + 1)),
+ (Decimal(3L, 1, Int.MaxValue), Decimal(2L, 1, Int.MaxValue - 1),
+ Decimal(3L, 1, Int.MaxValue)),
+ // These scale differences exceed Int's range, but their remainders
are representable.
+ (Decimal(0L, 1, Int.MinValue), Decimal(2L, 1, Int.MaxValue),
Decimal(0L, 1, -1)),
+ (Decimal(0L, 1, Int.MaxValue), Decimal(2L, 1, Int.MinValue),
+ Decimal(0L, 1, Int.MaxValue)),
+ (Decimal(1L, 1, Int.MaxValue), Decimal(2L, 1, Int.MinValue),
+ Decimal(1L, 1, Int.MaxValue))
+ ).foreach { case (left, right, expected) =>
+ assert((left % right).toJavaBigDecimal == expected.toJavaBigDecimal)
+ assert((-left % right).toJavaBigDecimal ==
(-expected).toJavaBigDecimal)
+ assert((left % -right).toJavaBigDecimal == expected.toJavaBigDecimal)
+ }
+ }
+ }
+
+ test("decimal remainder preserves huge trailing-zero quotients at extreme
negative scales") {
+ withSQLConf(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key ->
"true") {
+ for {
+ scale <- Seq(Int.MinValue, Int.MinValue + 1, Int.MinValue + 2)
+ leftSign <- Seq(1L, -1L)
+ rightSign <- Seq(1L, -1L)
+ } {
+ val left = Decimal(leftSign, 1, scale)
+ val right = Decimal(rightSign)
+ val result = left % right
+ assert(result.isZero)
+ assert(result.scale == scale)
+ }
+ }
+ }
+
+ test("decimal remainder propagates failure when both division attempts
fail") {
Review Comment:
**Nit (P3):** Test-only: `Decimal.%` itself behaves as the PR declares, but
this test no longer exercises the path its title names. It was written for the
first commit's unbounded exact retry. With the 76-digit bound, the estimate for
`Decimal(1L, 1, Int.MinValue) % Decimal(1L, 1, Int.MaxValue)` is `1 -
Int.MinValue + Int.MaxValue = 2^32`. `%` therefore rethrows the context failure
at the bound and never calls the exact `remainder(divisor)`. The two direct
intercepts above it only exercise JDK `BigDecimal`. So the test now duplicates
the rethrow check in `decimal remainder bounds exact retries for legacy
negative scales`, with a weaker assertion. Within the bound, practically sized
operands cannot make the exact retry fail, so there is no double-failure path
left to cover. The PR description's claim to cover "cases where both attempts
fail" is stale for the same reason.
There is also a coverage gap. Every rejection case in the new tests exceeds
76 through dividend scale alone: this test, and the bound test, whose divisor
`Decimal(3L)` has scale 0. Dropping or negating `+ divisor.scale()` in the
estimate would leave every new test green. Yet that term sets where legacy
negative-scale inputs are rejected. It also keeps out-of-range direct-API
divisors such as `Decimal("3E-100000000")` away from an exact retry that would
materialize a quotient of about 10^8 digits.
Suggested change: replace this test with a case that is rejected only
because of the divisor's scale. For example, `Decimal(1) % Decimal("3E-100")`
has an estimate of 101; without the term it would return `1E-100`. As the
legacy-scale bound test does, take the expected message from the 39-digit
context remainder on the same operands, assert that `%` rethrows it, and title
the test for that behavior.
**Verification:**
- **Regression:** With the divisor-scale term removed from or negated in the
retry estimate, the new divisor-scale bound case fails because the remainder
returns an exact value instead of rethrowing. With the term present, the case
passes, and the rethrown message equals the message of the 39-digit context
remainder for the same operands.
- **Behavior:** The existing decimal remainder coverage still passes
unchanged: wide-quotient results, preserved context-path results and scales,
legacy and extreme scales, and the dividend-side bound cases.
--
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]
