Skip to content

fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling#23898

Open
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:audit-pmod
Open

fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling#23898
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:audit-pmod

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

  • Closes #.

No issue tracked these three bugs; they were surfaced by an audit of pmod
against 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:

Rationale for this change

Spark computes pmod as ((r + n) % n) when r < 0, where r = a % n, using
Java 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 returns
2147483647.

The width rule is subtle. Java promotes byte and short operands to int,
so those widths cannot overflow, while int and long are evaluated at their
own width and wrap. Both halves are observable:

spark-sql> SELECT pmod(CAST(-1 AS INT), CAST(-2147483648 AS INT));
2147483647
spark-sql> SELECT pmod(CAST(-2 AS TINYINT), CAST(-128 AS TINYINT));
-2

The TINYINT case is what distinguishes the two rules: wrapping at Int8
would give 126.

A zero floating point divisor did not raise in ANSI mode. Arrow's rem
kernel only errors for integer and decimal types, so the float path returned
NaN where Spark raises.

-0.0 divisors and results were mishandled. Arrow's comparison kernels
order floats by total order, so -0.0 compared as less than 0.0 rather than
equal to it. A -0.0 divisor was therefore not recognised as a zero divisor,
and an unconditional add flattened -0.0 results to 0.0.

spark-sql> SELECT pmod(CAST(10.5 AS DOUBLE), CAST(-0.0 AS DOUBLE));
-- ANSI on:  [REMAINDER_BY_ZERO] Remainder by zero.
-- ANSI off: NULL

What changes are included in this PR?

Scoped to pmod. mod and the shared try_rem helper are untouched.

  • The shift is evaluated as ((r + n) % n) rather than (r + n). The second
    modulo is a no-op for a positive divisor but decides the negative case:
    pmod(-7, -3) is -1, not -4.
  • Int8 and Int16 are widened to Int32 for the shift, reproducing Java's
    numeric promotion; wider types use a wrapping add so overflow matches Spark.
  • The zero divisor check is applied to floats as well as integers and decimals.
    In ANSI mode a zero divisor raises; otherwise it yields NULL. The check is
    masked by the validity of the dividend, because Spark's pmod is null
    intolerant: a NULL dividend evaluates to NULL and never raises, even where
    the divisor on that row is zero.
  • The shift is selected only where r < 0, rather than adding zero elsewhere,
    so -0.0 results survive.
  • pmod no longer routes through try_rem.

Are these changes tested?

Yes. pmod.slt grows by 238 lines and the unit tests in modulus.rs by a
similar 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:

  • the full overflow boundary for Int8, Int16, Int32 and Int64,
    including i32::MIN % -1 and the pmod(-1, TYPE_MIN) family
  • zero and -0.0 divisors across integer, float and decimal types, in both
    ANSI modes
  • NULL dividend paired with a zero divisor, which must not raise under ANSI
  • negative divisors, which exercise the second modulo

Fail-before evidence: stashing only modulus.rs and re-running produces 11
errors naming exactly the added queries — four "expected to fail but succeeded"
for the ANSI and -0.0 cases, and seven overflow errors.

Verified with:

cargo fmt --all -- --check
cargo clippy --all-targets --all-features -- -D warnings
cargo test --test sqllogictests -- spark/math/pmod
cargo test -p datafusion-spark --lib modulus

All 60 files under test_files/spark/math/ pass, not only pmod.slt.

…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-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.43558% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.65%. Comparing base (8393fd3) to head (9903028).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/spark/src/function/math/modulus.rs 83.43% 7 Missing and 20 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spark sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants