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: [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]
