You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/07/25 19:00:26 UTC
[PR] fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling [datafusion]
andygrove opened a new pull request, #23898:
URL: https://github.com/apache/datafusion/pull/23898
## 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:
- https://github.com/apache/datafusion/issues/23894 — `mod` has the same zero
divisor and `-0.0` bugs fixed here for `pmod`
- https://github.com/apache/datafusion/issues/23895 — `pmod` returns a wider
decimal type than Spark
- https://github.com/apache/datafusion/issues/23896 — `pmod` rejects string
arguments Spark implicitly casts
- https://github.com/apache/datafusion/issues/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`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #23898:
URL: https://github.com/apache/datafusion/pull/23898#issuecomment-5529452229
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23898:
URL: https://github.com/apache/datafusion/pull/23898#issuecomment-6406191323
Marked as draft after 14 days without activity: failing CI checks.
Please address these items and mark **Ready for review** when ready.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #23898:
URL: https://github.com/apache/datafusion/pull/23898#issuecomment-5704175863
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23898:
URL: https://github.com/apache/datafusion/pull/23898#issuecomment-5080251952
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/23898?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `83.43558%` with `27 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.65%. Comparing base ([`8393fd3`](https://app.codecov.io/gh/apache/datafusion/commit/8393fd32b7152779886c2b0176fab12af0cbbe0c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9903028`](https://app.codecov.io/gh/apache/datafusion/commit/9903028fa1262c00599efab9215365037fab553d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/23898?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/spark/src/function/math/modulus.rs](https://app.codecov.io/gh/apache/datafusion/pull/23898?src=pr&el=tree&filepath=datafusion%2Fspark%2Fsrc%2Ffunction%2Fmath%2Fmodulus.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zcGFyay9zcmMvZnVuY3Rpb24vbWF0aC9tb2R1bHVzLnJz) | 83.43% | [7 Missing and 20 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/23898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/23898?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling [datafusion]
Posted by "amitvijapur (via GitHub)" <gi...@apache.org>.
amitvijapur commented on PR #23898:
URL: https://github.com/apache/datafusion/pull/23898#issuecomment-5603695583
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org