This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-23987-bf1c17ae6b41a0aa8fe95a7b20273aefb47d3b10 in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit 2da78d67ce6dc3e2975906b3ca60e35cd1d9afed Author: kid <[email protected]> AuthorDate: Sun Aug 16 01:23:51 2026 +0000 fix(spark): correct mod ANSI zero divisor and negative zero handling (#23987) ## Which issue does this PR close? - Closes #23894 ## Rationale for this change `datafusion-spark`'s `mod` routes through the shared `try_rem` helper in `datafusion/spark/src/function/math/modulus.rs`, which has two gaps in its zero-divisor handling (both verified against Spark 3.5.8 – 4.2.0 in the issue): 1. **ANSI mode, floating-point divisor.** `try_rem` delegates to Arrow's `rem` kernel 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 raises `REMAINDER_BY_ZERO` for a zero divisor of any numeric type: ```sql set datafusion.execution.enable_ansi_mode = true; SELECT mod(10.5::float8, 0.0::float8); -- NaN, Spark raises ``` 2. **`-0.0` divisor, both modes.** The legacy path nulls out zero divisors via `eq(right, 0)`, but Arrow's floating-point comparisons use a total order in which `-0.0` is distinct from `0.0`, so a `-0.0` divisor goes unrecognised. Spark's `isZero` is a numeric comparison and treats `-0.0` as zero: ```sql SELECT mod(10.5::float8, -0.0::float8); -- NaN, Spark returns NULL (legacy) ``` ## What changes are included in this PR? `try_rem` now detects zero divisors itself, mirroring the shape #23898 established for `pmod`: - A new `is_zero` helper counts `-0.0` as zero for the floating-point types (via a `negative_zero` companion, same as #23898). - In ANSI mode, any row with a zero divisor raises `ArrowError::DivideByZero` — the same error Arrow's `rem` already produces for integers today, so the message 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. - Both modes substitute NULL for zero divisors before calling Arrow's `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 `pmod` to no longer use `try_rem` and adds identical `is_zero`/`negative_zero` helpers. This PR is independent of it — `mod` is fixed either way — but whichever lands second should dedupe the helpers in a rebase. Until #23898 lands, `pmod` also picks up the `-0.0` and ANSI floating-point zero-divisor fixes through the shared helper. 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: - New unit tests in `modulus.rs`: ANSI floating-point zero divisor raises; `-0.0` divisor returns NULL in legacy mode and raises in ANSI mode; a NULL dividend with a zero divisor returns NULL in ANSI mode (integer and float). - New sqllogictest cases in `spark/math/mod.slt` covering the same behavior at SQL level. - Verified `cargo test -p datafusion-spark`, the `spark/math/mod.slt` and `spark/math/pmod.slt` sqllogictests, `./dev/rust_lint.sh`, and the extended workspace 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`/`pmod` functions: in ANSI mode a floating-point zero divisor now raises instead of returning NaN, and a `-0.0` divisor is treated as zero in both modes (NULL in legacy mode, an error in ANSI mode), matching Spark. No API changes. --- datafusion/spark/src/function/math/modulus.rs | 265 +++++++++++++++++++-- .../sqllogictest/test_files/spark/math/mod.slt | 31 +++ .../sqllogictest/test_files/spark/math/pmod.slt | 25 ++ 3 files changed, 302 insertions(+), 19 deletions(-) diff --git a/datafusion/spark/src/function/math/modulus.rs b/datafusion/spark/src/function/math/modulus.rs index 97f59c2cbb..c37513c12c 100644 --- a/datafusion/spark/src/function/math/modulus.rs +++ b/datafusion/spark/src/function/math/modulus.rs @@ -15,41 +15,77 @@ // specific language governing permissions and limitations // under the License. -use arrow::array::{Scalar, new_null_array}; +use arrow::array::{ArrayRef, BooleanArray, Scalar, new_null_array}; use arrow::compute::kernels::numeric::add; use arrow::compute::kernels::{ + boolean::{and, is_not_null, or}, cmp::{eq, lt}, - numeric::rem, + numeric::{neg, rem}, zip::zip, }; use arrow::datatypes::DataType; +use arrow::error::ArrowError; use datafusion_common::{Result, ScalarValue, assert_eq_or_internal_err}; use datafusion_expr::{ ColumnarValue, ScalarFunctionArgs, ScalarUDFImpl, Signature, Volatility, }; +/// Returns a one element array holding negative zero, for the floating point +/// types only. +/// +/// Arrow's comparison kernels order floating point values totally, so `-0.0` +/// compares as distinct from, and less than, `0.0`. Java, and therefore Spark, +/// treats `-0.0` as equal to zero. The helper below uses this to restore the +/// IEEE 754 answer. +fn negative_zero(data_type: &DataType) -> Result<Option<ArrayRef>> { + match data_type { + DataType::Float16 | DataType::Float32 | DataType::Float64 => { + let zero = ScalarValue::new_zero(data_type)?.to_array()?; + Ok(Some(neg(zero.as_ref())?)) + } + _ => Ok(None), + } +} + +/// Rows of `values` that equal zero, counting `-0.0` as zero. +fn is_zero(values: &ArrayRef) -> Result<BooleanArray> { + let zero = ScalarValue::new_zero(values.data_type())?.to_array()?; + let mask = eq(values, &Scalar::new(zero))?; + match negative_zero(values.data_type())? { + Some(negative_zero) => Ok(or(&mask, &eq(values, &Scalar::new(negative_zero))?)?), + None => Ok(mask), + } +} + /// Computes `rem(left, right)` with divide-by-zero handling. -/// In ANSI mode, any zero divisor causes an error. -/// In legacy mode (ANSI off), zero divisors are replaced with NULL before -/// computing the remainder, so those positions return NULL while others -/// compute normally. +/// In ANSI mode, a zero divisor of any numeric type causes an error, with +/// `-0.0` counting as zero; a row whose dividend is NULL never raises, to +/// match Spark's null-intolerant remainder. In legacy mode (ANSI off), zero +/// divisors are replaced with NULL before computing the remainder, so those +/// positions return NULL while others compute normally. fn try_rem( - left: &arrow::array::ArrayRef, - right: &arrow::array::ArrayRef, + left: &ArrayRef, + right: &ArrayRef, enable_ansi_mode: bool, -) -> Result<arrow::array::ArrayRef> { +) -> Result<ArrayRef> { + let divisor_is_zero = is_zero(right)?; + // Null out zero divisors so that the remainder kernels never see one: + // division by zero then returns NULL instead of erroring (integers) or + // returning NaN (floats). ANSI mode reports the error itself below, so + // this substitution is harmless on rows that must raise. + let null = Scalar::new(new_null_array(right.data_type(), 1)); + let safe_right = zip(&divisor_is_zero, &null, right)?; if enable_ansi_mode { - Ok(rem(left, right)?) - } else { - // In legacy mode, null out zero divisors so that division by zero - // returns NULL instead of erroring (integers) or returning NaN (floats). - let zero = ScalarValue::new_zero(right.data_type())?.to_array()?; - let zero = Scalar::new(zero); - let null = Scalar::new(new_null_array(right.data_type(), 1)); - let is_zero = eq(right, &zero)?; - let safe_right = zip(&is_zero, &null, right)?; - Ok(rem(left, &safe_right)?) + // Spark's remainder expressions are null intolerant, so a row whose + // dividend is NULL evaluates to NULL and never raises, even when the + // divisor on that row is zero. Mask the check by the validity of the + // dividend to match. + let raises = and(&divisor_is_zero, &is_not_null(left.as_ref())?)?; + if raises.iter().flatten().any(|raises| raises) { + return Err(ArrowError::DivideByZero.into()); + } } + Ok(rem(left, &safe_right)?) } /// Spark-compatible `mod` function @@ -418,6 +454,101 @@ mod test { assert!(result.is_err()); } + #[test] + fn test_mod_zero_division_ansi_float() { + // In ANSI mode a zero divisor of any numeric type must raise, + // including floating point, where Arrow's `rem` follows IEEE 754 + // and quietly returns NaN (#23894) + let left = Float64Array::from(vec![Some(10.5), Some(7.2)]); + let right = Float64Array::from(vec![Some(0.0), Some(2.0)]); + + let left_value = ColumnarValue::Array(Arc::new(left)); + let right_value = ColumnarValue::Array(Arc::new(right)); + + let result = spark_mod(&[left_value, right_value], true); + assert!(result.is_err()); + } + + #[test] + fn test_mod_negative_zero_divisor_legacy() { + // `-0.0` counts as a zero divisor, so it returns NULL in legacy + // mode rather than NaN (#23894) + let left = Float64Array::from(vec![Some(10.5), Some(7.5)]); + let right = Float64Array::from(vec![Some(-0.0), Some(2.0)]); + + let left_value = ColumnarValue::Array(Arc::new(left)); + let right_value = ColumnarValue::Array(Arc::new(right)); + + let result = spark_mod(&[left_value, right_value], false).unwrap(); + + if let ColumnarValue::Array(result_array) = result { + let result_float64 = result_array + .as_any() + .downcast_ref::<Float64Array>() + .unwrap(); + assert!(result_float64.is_null(0)); // 10.5 % -0.0 = NULL + assert_eq!(result_float64.value(1), 1.5); // 7.5 % 2.0 = 1.5 + } else { + panic!("Expected array result"); + } + } + + #[test] + fn test_mod_negative_zero_divisor_ansi() { + // `-0.0` counts as a zero divisor, so it raises in ANSI mode (#23894) + let left = Float64Array::from(vec![Some(10.5)]); + let right = Float64Array::from(vec![Some(-0.0)]); + + let left_value = ColumnarValue::Array(Arc::new(left)); + let right_value = ColumnarValue::Array(Arc::new(right)); + + let result = spark_mod(&[left_value, right_value], true); + assert!(result.is_err()); + } + + #[test] + fn test_mod_zero_division_ansi_null_dividend() { + // Spark's remainder expressions are null intolerant: a NULL dividend + // short-circuits to NULL before the divisor is validated, so a zero + // divisor on such a row must not raise, even in ANSI mode (#23894) + let left = Int32Array::from(vec![None, Some(10)]); + let right = Int32Array::from(vec![Some(0), Some(3)]); + + let left_value = ColumnarValue::Array(Arc::new(left)); + let right_value = ColumnarValue::Array(Arc::new(right)); + + let result = spark_mod(&[left_value, right_value], true).unwrap(); + + if let ColumnarValue::Array(result_array) = result { + let result_int32 = + result_array.as_any().downcast_ref::<Int32Array>().unwrap(); + assert!(result_int32.is_null(0)); // NULL % 0 = NULL (no error) + assert_eq!(result_int32.value(1), 1); // 10 % 3 = 1 + } else { + panic!("Expected array result"); + } + + // Same for floating point + let left = Float64Array::from(vec![None, Some(10.5)]); + let right = Float64Array::from(vec![Some(0.0), Some(2.0)]); + + let left_value = ColumnarValue::Array(Arc::new(left)); + let right_value = ColumnarValue::Array(Arc::new(right)); + + let result = spark_mod(&[left_value, right_value], true).unwrap(); + + if let ColumnarValue::Array(result_array) = result { + let result_float64 = result_array + .as_any() + .downcast_ref::<Float64Array>() + .unwrap(); + assert!(result_float64.is_null(0)); // NULL % 0.0 = NULL (no error) + assert!((result_float64.value(1) - 0.5).abs() < f64::EPSILON); // 10.5 % 2.0 = 0.5 + } else { + panic!("Expected array result"); + } + } + // PMOD tests #[test] fn test_pmod_int32() { @@ -645,6 +776,102 @@ mod test { assert!(result.is_err()); } + #[test] + fn test_pmod_zero_division_ansi_float() { + // pmod routes through `try_rem` twice, so it needs the same coverage + // as mod: in ANSI mode a zero divisor of any numeric type must + // raise, including floating point, where Arrow's `rem` follows + // IEEE 754 and quietly returns NaN (#23894) + let left = Float64Array::from(vec![Some(10.5), Some(7.2)]); + let right = Float64Array::from(vec![Some(0.0), Some(2.0)]); + + let left_value = ColumnarValue::Array(Arc::new(left)); + let right_value = ColumnarValue::Array(Arc::new(right)); + + let result = spark_pmod(&[left_value, right_value], true); + assert!(result.is_err()); + } + + #[test] + fn test_pmod_negative_zero_divisor_legacy() { + // `-0.0` counts as a zero divisor, so it returns NULL in legacy + // mode rather than NaN (#23894) + let left = Float64Array::from(vec![Some(10.5), Some(-7.5)]); + let right = Float64Array::from(vec![Some(-0.0), Some(2.0)]); + + let left_value = ColumnarValue::Array(Arc::new(left)); + let right_value = ColumnarValue::Array(Arc::new(right)); + + let result = spark_pmod(&[left_value, right_value], false).unwrap(); + + if let ColumnarValue::Array(result_array) = result { + let result_float64 = result_array + .as_any() + .downcast_ref::<Float64Array>() + .unwrap(); + assert!(result_float64.is_null(0)); // 10.5 pmod -0.0 = NULL + assert!((result_float64.value(1) - 0.5).abs() < f64::EPSILON); // -7.5 pmod 2.0 = 0.5 + } else { + panic!("Expected array result"); + } + } + + #[test] + fn test_pmod_negative_zero_divisor_ansi() { + // `-0.0` counts as a zero divisor, so it raises in ANSI mode (#23894) + let left = Float64Array::from(vec![Some(10.5)]); + let right = Float64Array::from(vec![Some(-0.0)]); + + let left_value = ColumnarValue::Array(Arc::new(left)); + let right_value = ColumnarValue::Array(Arc::new(right)); + + let result = spark_pmod(&[left_value, right_value], true); + assert!(result.is_err()); + } + + #[test] + fn test_pmod_zero_division_ansi_null_dividend() { + // Spark's remainder expressions are null intolerant: a NULL dividend + // short-circuits to NULL before the divisor is validated, so a zero + // divisor on such a row must not raise, even in ANSI mode (#23894) + let left = Int32Array::from(vec![None, Some(10)]); + let right = Int32Array::from(vec![Some(0), Some(3)]); + + let left_value = ColumnarValue::Array(Arc::new(left)); + let right_value = ColumnarValue::Array(Arc::new(right)); + + let result = spark_pmod(&[left_value, right_value], true).unwrap(); + + if let ColumnarValue::Array(result_array) = result { + let result_int32 = + result_array.as_any().downcast_ref::<Int32Array>().unwrap(); + assert!(result_int32.is_null(0)); // NULL pmod 0 = NULL (no error) + assert_eq!(result_int32.value(1), 1); // 10 pmod 3 = 1 + } else { + panic!("Expected array result"); + } + + // Same for floating point + let left = Float64Array::from(vec![None, Some(10.5)]); + let right = Float64Array::from(vec![Some(0.0), Some(2.0)]); + + let left_value = ColumnarValue::Array(Arc::new(left)); + let right_value = ColumnarValue::Array(Arc::new(right)); + + let result = spark_pmod(&[left_value, right_value], true).unwrap(); + + if let ColumnarValue::Array(result_array) = result { + let result_float64 = result_array + .as_any() + .downcast_ref::<Float64Array>() + .unwrap(); + assert!(result_float64.is_null(0)); // NULL pmod 0.0 = NULL (no error) + assert!((result_float64.value(1) - 0.5).abs() < f64::EPSILON); // 10.5 pmod 2.0 = 0.5 + } else { + panic!("Expected array result"); + } + } + #[test] fn test_pmod_negative_divisor() { // PMOD with negative divisor should still work like regular mod diff --git a/datafusion/sqllogictest/test_files/spark/math/mod.slt b/datafusion/sqllogictest/test_files/spark/math/mod.slt index 8229bb0651..b9a8499939 100644 --- a/datafusion/sqllogictest/test_files/spark/math/mod.slt +++ b/datafusion/sqllogictest/test_files/spark/math/mod.slt @@ -160,6 +160,18 @@ SELECT MOD(10.5::float8, 0.0::float8) as mod_div_zero_float; ---- NULL +# A negative zero divisor counts as zero and returns NULL in legacy mode +query R +SELECT MOD(10.5::float8, -0.0::float8) as mod_div_zero_neg_float; +---- +NULL + +# A NULL dividend evaluates to NULL regardless of the divisor +query I +SELECT MOD(NULL::int, 0::int) as mod_null_dividend_legacy; +---- +NULL + # Division by zero errors in ANSI mode statement ok set datafusion.execution.enable_ansi_mode = true; @@ -170,6 +182,25 @@ SELECT MOD(10::int, 0::int); statement error DataFusion error: Arrow error: Divide by zero error SELECT MOD(-7::int, 0::int); +# A zero divisor of any numeric type raises, including floating point +statement error DataFusion error: Arrow error: Divide by zero error +SELECT MOD(10.5::float8, 0.0::float8); + +# A negative zero divisor counts as zero and raises +statement error DataFusion error: Arrow error: Divide by zero error +SELECT MOD(10.5::float8, -0.0::float8); + +# A NULL dividend short-circuits to NULL before the divisor is validated +query I +SELECT MOD(NULL::int, 0::int) as mod_null_dividend_ansi; +---- +NULL + +query R +SELECT MOD(NULL::float8, 0.0::float8) as mod_null_dividend_ansi_float; +---- +NULL + statement ok set datafusion.execution.enable_ansi_mode = false; diff --git a/datafusion/sqllogictest/test_files/spark/math/pmod.slt b/datafusion/sqllogictest/test_files/spark/math/pmod.slt index aa4a197ba4..1165fdfba3 100644 --- a/datafusion/sqllogictest/test_files/spark/math/pmod.slt +++ b/datafusion/sqllogictest/test_files/spark/math/pmod.slt @@ -74,6 +74,12 @@ SELECT pmod(-7::int, 0::int) as pmod_zero_3; ---- NULL +# A negative zero divisor counts as zero and returns NULL in legacy mode +query R +SELECT pmod(10.5::float8, -0.0::float8) as pmod_div_zero_neg_float; +---- +NULL + # Division by zero errors in ANSI mode statement ok set datafusion.execution.enable_ansi_mode = true; @@ -84,6 +90,25 @@ SELECT pmod(10::int, 0::int); statement error DataFusion error: Arrow error: Divide by zero error SELECT pmod(-7::int, 0::int); +# A zero divisor of any numeric type raises, including floating point +statement error DataFusion error: Arrow error: Divide by zero error +SELECT pmod(10.5::float8, 0.0::float8); + +# A negative zero divisor counts as zero and raises +statement error DataFusion error: Arrow error: Divide by zero error +SELECT pmod(10.5::float8, -0.0::float8); + +# A NULL dividend short-circuits to NULL before the divisor is validated +query I +SELECT pmod(NULL::int, 0::int) as pmod_null_dividend_ansi; +---- +NULL + +query R +SELECT pmod(NULL::float8, 0.0::float8) as pmod_null_dividend_ansi_float; +---- +NULL + statement ok set datafusion.execution.enable_ansi_mode = false; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
