mbutrovich commented on code in PR #5081:
URL: https://github.com/apache/datafusion-comet/pull/5081#discussion_r3683272168
##########
native/spark-expr/src/math_funcs/modulo_expr.rs:
##########
@@ -441,4 +520,126 @@ mod tests {
verify_result(modulo_expr, batch, fail_on_error,
Some(expected_result));
})
}
+
+ fn run_float_modulo<T: ArrowPrimitiveType>(
Review Comment:
All six new float tests go through `run_float_modulo`, which builds both
operands as `Column` expressions, so `spark_modulo` always sees
`(ColumnarValue::Array, ColumnarValue::Array)`. The `Scalar/Scalar`,
`Array/Scalar`, and `Scalar/Array` match arms in
`check_float_remainder_by_zero` are not exercised by any of these tests.
Could you add a case that passes at least one operand as a `Literal`, the
way `test_division_by_zero_with_complex_int_expr` does with
`Literal::new(ScalarValue::Int32(Some(0)))`?
##########
native/spark-expr/src/math_funcs/modulo_expr.rs:
##########
@@ -63,6 +71,77 @@ pub fn spark_modulo(args: &[ColumnarValue], fail_on_error:
bool) -> Result<Colum
}
}
+/// Returns an error if any row has a non-null dividend paired with a zero
divisor. Only
+/// meant to be called with floating point operands — Spark treats `-0.0` as
zero here,
+/// which falls out naturally from IEEE 754 equality.
+fn check_float_remainder_by_zero(lhs: &ColumnarValue, rhs: &ColumnarValue) ->
Result<()> {
+ let is_zero_float_scalar = |sv: &ScalarValue| match sv {
+ ScalarValue::Float32(Some(v)) => *v == 0.0,
+ ScalarValue::Float64(Some(v)) => *v == 0.0,
+ _ => false,
+ };
+
+ match (lhs, rhs) {
+ (ColumnarValue::Scalar(l), ColumnarValue::Scalar(r)) => {
+ if !l.is_null() && is_zero_float_scalar(r) {
+ return Err(remainder_by_zero_error().into());
+ }
+ }
+ (ColumnarValue::Array(l_arr), ColumnarValue::Scalar(r)) => {
+ if is_zero_float_scalar(r) && l_arr.null_count() != l_arr.len() {
+ return Err(remainder_by_zero_error().into());
+ }
+ }
+ (ColumnarValue::Scalar(l), ColumnarValue::Array(r_arr)) => {
+ if !l.is_null() && float_array_has_zero(r_arr, None) {
+ return Err(remainder_by_zero_error().into());
+ }
+ }
+ (ColumnarValue::Array(l_arr), ColumnarValue::Array(r_arr)) => {
+ if float_array_has_zero(r_arr, Some(l_arr.as_ref())) {
+ return Err(remainder_by_zero_error().into());
+ }
+ }
+ }
+ Ok(())
+}
+
+/// Returns true if `divisor` contains any non-null zero. When `dividend_mask`
is provided,
+/// positions where `dividend_mask` is null are skipped (they will produce
null results and
+/// must not raise an error).
+fn float_array_has_zero(divisor: &ArrayRef, dividend_mask: Option<&dyn Array>)
-> bool {
+ match divisor.data_type() {
+ DataType::Float32 => scan_float_array::<Float32Type>(divisor,
dividend_mask),
+ DataType::Float64 => scan_float_array::<Float64Type>(divisor,
dividend_mask),
+ _ => false,
+ }
+}
+
+fn scan_float_array<T: ArrowPrimitiveType>(
+ divisor: &ArrayRef,
+ dividend_mask: Option<&dyn Array>,
+) -> bool
+where
+ T::Native: Default + PartialEq,
+{
+ let arr = divisor.as_primitive::<T>();
+ let zero = T::Native::default();
+ for i in 0..arr.len() {
+ if arr.is_null(i) {
+ continue;
+ }
+ if let Some(mask) = dividend_mask {
+ if mask.is_null(i) {
+ continue;
+ }
+ }
+ if arr.value(i) == zero {
+ return true;
+ }
+ }
+ false
+}
Review Comment:
Thanks for tracking down the root cause here. The `mod_wrapping` vs
`mod_checked` split in arrow-arith explains exactly why the existing error
branch was unreachable for floats.
`check_float_remainder_by_zero` and its helpers (`float_array_has_zero`,
`scan_float_array`) reimplement something arrow-rs already provides.
`ArrowNativeTypeOp::mod_checked` for `f32`/`f64` already returns `DivideByZero`
when the divisor `is_zero()`, and computes `self % rhs` otherwise, so `-0.0` is
handled the same way there as it is in this hand-written check.
`checked_arithmetic.rs` already takes this route for the equivalent
float-divide-by-zero problem: it calls
`arrow::compute::kernels::arity::try_binary` with `div_checked` instead of
relying on `arrow::compute::kernels::numeric::div`, because that kernel has the
same wrapping-only behavior for floats that `rem()` has here. Would
`try_binary` with `mod_checked` work for this fix too? Since `mod_checked` for
floats can only fail with `DivideByZero` (there is no overflow case for
remainder), any `Err` from that call maps directly to
`remainder_by_zero_error()`. `try_binary` also only invokes the closure on rows
where both operands are valid, so the null-dividend skip logic in
`float_array_has_zero` and `scan_float_array` would not be needed. This would
also remove the extra full pass over the divisor array that the current code
does before `rem()` runs its own pass, on every ANSI-mode float remainder call,
including the common case with no zero divisor.
Rough sketch of what this could look like in place of
`check_float_remainder_by_zero`, `float_array_has_zero`, and `scan_float_array`
(not compiled, just to make the suggestion concrete):
```rust
if fail_on_error && matches!(right_data_type, DataType::Float32 |
DataType::Float64) {
return checked_float_modulo(lhs, rhs, &right_data_type);
}
match apply(lhs, rhs, rem) {
Ok(result) => Ok(result),
Err(e) if e.to_string().contains("Divide by zero") && fail_on_error => {
Err(remainder_by_zero_error().into())
}
Err(e) => Err(e),
}
```
```rust
fn checked_float_modulo(
lhs: &ColumnarValue,
rhs: &ColumnarValue,
data_type: &DataType,
) -> Result<ColumnarValue> {
let len = match (lhs, rhs) {
(ColumnarValue::Array(a), _) | (_, ColumnarValue::Array(a)) =>
a.len(),
_ => 1,
};
let l_arr = match lhs {
ColumnarValue::Array(a) => Arc::clone(a),
ColumnarValue::Scalar(s) => s.to_array_of_size(len)?,
};
let r_arr = match rhs {
ColumnarValue::Array(a) => Arc::clone(a),
ColumnarValue::Scalar(s) => s.to_array_of_size(len)?,
};
let result: ArrayRef = match data_type {
DataType::Float32 => Arc::new(
try_binary::<_, _, _, Float32Type>(
l_arr.as_primitive::<Float32Type>(),
r_arr.as_primitive::<Float32Type>(),
|l, r| l.mod_checked(r),
)
.map_err(|_| remainder_by_zero_error())?,
),
DataType::Float64 => Arc::new(
try_binary::<_, _, _, Float64Type>(
l_arr.as_primitive::<Float64Type>(),
r_arr.as_primitive::<Float64Type>(),
|l, r| l.mod_checked(r),
)
.map_err(|_| remainder_by_zero_error())?,
),
_ => unreachable!(),
};
Ok(ColumnarValue::Array(result))
}
```
Needs `use arrow::array::{ArrowNativeTypeOp, AsArray}` and `use
arrow::compute::kernels::arity::try_binary`, both already imported in
`checked_arithmetic.rs` for the same purpose.
##########
spark/src/test/resources/sql-tests/expressions/math/arithmetic_ansi.sql:
##########
@@ -147,6 +147,37 @@ SELECT c div d FROM ansi_div_zero
query expect_error(BY_ZERO)
SELECT c % d FROM ansi_div_zero
+-- ============================================================================
+-- Float/Double remainder by zero (issue #5067)
+-- Spark 4.1 throws REMAINDER_BY_ZERO for floating-point x % 0 as well.
+-- ============================================================================
+
+statement
+CREATE TABLE ansi_float_div_zero(a float, b float, c double, d double) USING
parquet
+
+statement
+INSERT INTO ansi_float_div_zero VALUES (3.0, 0.0, 3.0, 0.0), (1.0, -0.0, 1.0,
-0.0)
+
+-- float column % 0 should throw
+query expect_error(BY_ZERO)
+SELECT a % b FROM ansi_float_div_zero WHERE b = 0.0
+
+-- double column % 0 should throw
+query expect_error(BY_ZERO)
+SELECT c % d FROM ansi_float_div_zero WHERE d = 0.0
+
+-- literal double % 0.0 should throw
+query expect_error(BY_ZERO)
+SELECT CAST(1.0 AS DOUBLE) % CAST(0.0 AS DOUBLE)
Review Comment:
This expression is fully literal, so Spark's own `ConstantFolding` rule
evaluates and throws on it during optimization, before the query reaches
Comet's native `spark_modulo`. The same is true for the `-0.0` and float cases
on lines 175 and 179. This mirrors an existing pattern already in this file
(`SELECT 1 % 0` above), so it is not a new issue, but it means these three
cases do not exercise this PR's new code.
Could you add a case like `SELECT a % 0.0 FROM ansi_float_div_zero` (column
dividend, literal zero divisor)? That query is not foldable, since `a` is a
column, so it would actually reach Comet's native path at execution time and
cover the `Array/Scalar` branch that the Rust unit tests also miss.
--
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]
