fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling#23898
Open
andygrove wants to merge 1 commit into
Open
fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling#23898andygrove wants to merge 1 commit into
andygrove wants to merge 1 commit into
Conversation
…o handling Audit `pmod` against Apache Spark 4.2.0 and fix three divergences. Spark evaluates `(r + n) % n` with Java arithmetic, which wraps around for `int` and `long` and promotes `byte` and `short` to `int`. Arrow's checked `add` reported an overflow instead, so `pmod(-1, -2147483648)` errored where Spark returns `2147483647`. Use `add_wrapping`, and widen the narrow integer types before the addition so Java's operand promotion is reproduced. Arrow's `rem` only reports division by zero for integer and decimal types, so in ANSI mode a floating point zero divisor quietly produced `NaN` where Spark raises. Check the divisor directly, masked by the validity of the dividend so the null intolerant short circuit is preserved and `pmod(NULL, 0)` stays NULL. Arrow's comparison kernels order floating point values totally, so `-0.0` was neither recognised as a zero divisor nor kept out of the negative branch. Treat `-0.0` as zero when testing the divisor, and select the adjusted value only where the remainder is negative so a `-0.0` result keeps its sign. `pmod` no longer shares `try_rem` with `mod`, whose behavior is unchanged.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23898 +/- ##
==========================================
- Coverage 80.65% 80.65% -0.01%
==========================================
Files 1092 1092
Lines 371106 371260 +154
Branches 371106 371260 +154
==========================================
+ Hits 299323 299445 +122
- Misses 53937 53947 +10
- Partials 17846 17868 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
No issue tracked these three bugs; they were surfaced by an audit of
pmodagainst the Spark source and a live Spark 4.2.0. The divergences this PR does
not fix were filed by that audit and are referenced from the test file:
modhas the same zerodivisor and
-0.0bugs fixed here forpmodpmodreturns a widerdecimal type than Spark
pmodrejects stringarguments Spark implicitly casts
message is reported instead of Spark's
REMAINDER_BY_ZERORationale for this change
Spark computes
pmodas((r + n) % n)whenr < 0, wherer = a % n, usingJava arithmetic. Three divergences follow from not reproducing that exactly.
Integer overflow where Spark wraps. The shift used a checked add, so
pmod(-1, -2147483648)raised a hard error. Java wraps, and Spark returns2147483647.The width rule is subtle. Java promotes
byteandshortoperands toint,so those widths cannot overflow, while
intandlongare evaluated at theirown width and wrap. Both halves are observable:
The
TINYINTcase is what distinguishes the two rules: wrapping atInt8would give
126.A zero floating point divisor did not raise in ANSI mode. Arrow's
remkernel only errors for integer and decimal types, so the float path returned
NaNwhere Spark raises.-0.0divisors and results were mishandled. Arrow's comparison kernelsorder floats by total order, so
-0.0compared as less than0.0rather thanequal to it. A
-0.0divisor was therefore not recognised as a zero divisor,and an unconditional add flattened
-0.0results to0.0.What changes are included in this PR?
Scoped to
pmod.modand the sharedtry_remhelper are untouched.((r + n) % n)rather than(r + n). The secondmodulo is a no-op for a positive divisor but decides the negative case:
pmod(-7, -3)is-1, not-4.Int8andInt16are widened toInt32for the shift, reproducing Java'snumeric promotion; wider types use a wrapping add so overflow matches Spark.
In ANSI mode a zero divisor raises; otherwise it yields NULL. The check is
masked by the validity of the dividend, because Spark's
pmodis nullintolerant: a NULL dividend evaluates to NULL and never raises, even where
the divisor on that row is zero.
r < 0, rather than adding zero elsewhere,so
-0.0results survive.pmodno longer routes throughtry_rem.Are these changes tested?
Yes.
pmod.sltgrows by 238 lines and the unit tests inmodulus.rsby asimilar amount. Every expected value was observed by running the query against
a local
pyspark==4.2.0, not derived from reading the Spark source.Coverage crosses the argument shapes with NULL handling and with both ANSI
modes, rather than testing each axis alone. Added specifically:
Int8,Int16,Int32andInt64,including
i32::MIN % -1and thepmod(-1, TYPE_MIN)family-0.0divisors across integer, float and decimal types, in bothANSI modes
Fail-before evidence: stashing only
modulus.rsand re-running produces 11errors naming exactly the added queries — four "expected to fail but succeeded"
for the ANSI and
-0.0cases, and seven overflow errors.Verified with:
All 60 files under
test_files/spark/math/pass, not onlypmod.slt.