fix(spark): correct mod ANSI zero divisor and negative zero handling - #23987
Open
u70b3 wants to merge 1 commit into
Open
fix(spark): correct mod ANSI zero divisor and negative zero handling#23987u70b3 wants to merge 1 commit into
u70b3 wants to merge 1 commit into
Conversation
In ANSI mode a floating-point zero divisor quietly returned NaN because Arrow's rem only reports division by zero for integer and decimal types, and a -0.0 divisor went unrecognised in both modes because Arrow's floating-point comparisons order totally, so -0.0 is distinct from 0.0. try_rem now detects zero divisors itself via an is_zero helper that counts -0.0 as zero, raises for a zero divisor of any numeric type in ANSI mode (masked by the validity of the dividend, since Spark's remainder is null intolerant), and substitutes NULL for zero divisors before calling Arrow's rem so the kernel never sees one. Closes apache#23894.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23987 +/- ##
==========================================
- Coverage 80.75% 80.75% -0.01%
==========================================
Files 1096 1096
Lines 373588 373660 +72
Branches 373588 373660 +72
==========================================
+ Hits 301687 301740 +53
- Misses 53898 53907 +9
- Partials 18003 18013 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
datafusion-spark'smodroutes through the sharedtry_remhelper indatafusion/spark/src/function/math/modulus.rs, which has two gaps in itszero-divisor handling (both verified against Spark 3.5.8 – 4.2.0 in the issue):
ANSI mode, floating-point divisor.
try_remdelegates to Arrow'sremkernel in ANSI mode, but Arrow only reports division by zero for integer and
decimal types. Floating-point divisors follow IEEE 754 and quietly produce
NaN, while Spark raisesREMAINDER_BY_ZEROfor a zero divisor of anynumeric type:
-0.0divisor, both modes. The legacy path nulls out zero divisors viaeq(right, 0), but Arrow's floating-point comparisons use a total order inwhich
-0.0is distinct from0.0, so a-0.0divisor goes unrecognised.Spark's
isZerois a numeric comparison and treats-0.0as zero:What changes are included in this PR?
try_remnow detects zero divisors itself, mirroring the shape #23898established for
pmod:is_zerohelper counts-0.0as zero for the floating-point types(via a
negative_zerocompanion, same as fix(spark): correct pmod overflow, ANSI zero divisor and negative zero handling #23898).ArrowError::DivideByZero— the same error Arrow's
remalready produces for integers today, so themessage stays uniform across types. The check is masked by the validity of
the dividend because Spark's remainder expressions are null intolerant: a
NULL dividend short-circuits to NULL before the divisor is validated, so
mod(NULL, 0)must return NULL rather than raise.rem,so the kernel never sees a zero divisor: legacy mode gets NULLs, and ANSI
mode has already raised on the rows that required it.
Note on overlap with #23898: that PR rewrites
pmodto no longer usetry_remand adds identicalis_zero/negative_zerohelpers. This PR isindependent of it —
modis fixed either way — but whichever lands secondshould dedupe the helpers in a rebase. Until #23898 lands,
pmodalso picksup the
-0.0and ANSI floating-point zero-divisor fixes through the sharedhelper.
Out of scope: #23897 (reproducing Spark's exact ANSI error text) is a
repository-wide error-message policy question, as noted in that issue.
Are these changes tested?
Yes:
modulus.rs: ANSI floating-point zero divisor raises;-0.0divisor returns NULL in legacy mode and raises in ANSI mode; a NULLdividend with a zero divisor returns NULL in ANSI mode (integer and float).
spark/math/mod.sltcovering the same behavior atSQL level.
cargo test -p datafusion-spark, thespark/math/mod.sltandspark/math/pmod.sltsqllogictests,./dev/rust_lint.sh, and the extendedworkspace suite (ci profile with
avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption)— all green.
Are there any user-facing changes?
Only the bug fixes, and only for the Spark
mod/pmodfunctions: in ANSImode a floating-point zero divisor now raises instead of returning NaN, and a
-0.0divisor is treated as zero in both modes (NULL in legacy mode, an errorin ANSI mode), matching Spark. No API changes.