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