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

[andygrove]

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:

• [Bug] mod returns NaN instead of raising for a zero floating point divisor in ANSI mode, and ignores -0.0 divisors #23894 — mod has the same zero
  divisor and -0.0 bugs fixed here for pmod
• [Bug] pmod returns a wider decimal type than Spark #23895 — pmod returns a wider
  decimal type than Spark
• [Bug] pmod rejects string arguments that Spark implicitly casts to a numeric type #23896 — pmod rejects string
  arguments Spark implicitly casts
• [Bug] pmod and mod report Arrow's divide-by-zero message instead of Spark's REMAINDER_BY_ZERO text #23897 — Arrow's divide-by-zero
  message is reported instead of Spark's REMAINDER_BY_ZERO

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.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 86.27451% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.93%. Comparing base (140c7c5) to head (96c88e1).

Files with missing lines	Patch %	Lines
datafusion/spark/src/function/math/modulus.rs	86.27%	7 Missing and 14 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #23898    +/-   ##
========================================
  Coverage   81.93%   81.93%            
========================================
  Files        1136     1136            
  Lines      429152   429299   +147     
  Branches   429152   429299   +147     
========================================
+ Hits       351633   351757   +124     
- Misses      56475    56488    +13     
- Partials    21044    21054    +10     

☔ 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.

[andygrove]

Rebased onto main. The conflict overlapped with #23987, so noting the resolution here.

#23987 fixed the ANSI zero divisor and -0.0 handling for mod while this branch was open, by rewriting the shared try_rem and adding negative_zero and is_zero. This branch had developed its own versions of the same three things for pmod. The overlap is resolved in favor of main:

• try_rem — main's version is kept. It handles the zero divisor of any numeric type, treats -0.0 as zero, and masks the ANSI check by the validity of the dividend. The version on this branch was the older one that delegated straight to Arrow's rem under ANSI. mod is therefore untouched by this PR.
• is_zero — byte-identical in both; main's is kept.
• negative_zero — same implementation. Kept main's, with the doc comment reworded to cover is_negative too.

What remains specific to this branch is the Java overflow and operand promotion handling (add_wrapping plus widening Int8/Int16), preserving the sign of a -0.0 result by selecting the adjusted value only where the remainder is negative, and the pmod test coverage.

One correction while rebasing: the doc comment on spark_pmod said it does not share try_rem because it needs to treat -0.0 as a zero divisor and raise on float zero divisors under ANSI. Since #23987 that is no longer a difference. The reason that still holds is that pmod needs the zero-divisor-masked divisor again for the (r + n) % n step, which try_rem does not return, so the comment now says that instead.

Both test suites are kept in full — 29 modulus unit tests, including main's *_negative_zero_divisor_*, *_ansi_float and *_ansi_null_dividend cases alongside this branch's test_pmod_integer_boundaries, test_pmod_negative_zero_result and test_pmod_zero_divisor_by_type. In pmod.slt, main's legacy-mode -0.0 case was not covered here and is kept.

Verified on the rebased branch: datafusion-spark 282 unit tests pass, and all 244 spark/ slt files pass. cargo fmt and cargo clippy -p datafusion-spark --all-targets -D warnings are clean.

[amitvijapur]

Heads up @andygrove — this now conflicts with main, and #24409 landing is the cause, so apologies for the extra work.

I test-merged it against current main (76 commits behind). Conflicts in datafusion/spark/src/function/math/modulus.rs and datafusion/sqllogictest/test_files/spark/math/pmod.slt.

The two changes look complementary rather than overlapping. #24409 added pmod_decimal_result_type (Spark's Remainder rule for the decimal result type) plus null passthrough for the uncoerced DataType::Null that Coercible lets through, per #19458. This PR adds the ANSI zero-divisor raise masked by dividend validity, is_negative with -0.0 handling, and the Java integer-promotion arithmetic. Nothing in one subsumes the other.

One thing worth watching in the resolution. This PR uses add_wrapping deliberately, so pmod(-1, -2147483648) yields 2147483647 the way Spark does. Main now imports plain arrow::compute::kernels::numeric::add and calls it at modulus.rs:172. A resolution that takes main's import block wholesale drops the wrapping behaviour, and I don't think any existing test catches that, so it would go silently. The Int8/Int16 widening to Int32 has the same property.

The other two conflicts are mechanical: the import block, and is_negative versus pmod_decimal_result_type landing at the same insertion point.

Happy to push the rebase myself if that's useful, since my PR caused it — just say the word and I'll open it against your branch. Otherwise this is just a flag.

Disclosure: written with AI assistance, reviewed by me before posting.

[andygrove]

Thanks @amitvijapur, merged main in. The resolution keeps #24409's pmod_decimal_result_type, the DataType::Null passthrough and the decimal narrowing cast, and keeps add_wrapping plus the Int8/Int16 widening for the (r + n) % n step, so main's plain add import is dropped. The overflow cases are covered by test_pmod_integer_boundaries and the pmod(-1, -2147483648) family in pmod.slt, so losing the wrapping behavior would now fail tests. The branch's unit tests were updated for the new result_type parameter. All datafusion-spark unit tests and all 244 spark/ slt files pass.