andygrove commented on code in PR #5081:
URL: https://github.com/apache/datafusion-comet/pull/5081#discussion_r3684143284
##########
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:
Good call — this is much better. Done in 6c815fea.
I confirmed both halves of the suggestion against the pinned arrow version
before switching:
- arrow-array 59.1.0 `src/arithmetic.rs:353` — float `mod_checked` is `if
rhs.is_zero() { Err(DivideByZero) } else { Ok(self % rhs) }`, and `is_zero` for
floats is `self == $zero`, so `-0.0` errors just like the hand-written check
did.
- arrow-arith 59.1.0 `src/arity.rs:283` — when either input has nulls,
`try_binary` runs the closure under `nulls.try_for_each_valid_idx`, so it only
touches rows valid in both operands. That makes the null-dividend skip logic
unnecessary, as you said.
So `check_float_remainder_by_zero`, `float_array_has_zero`, and
`scan_float_array` are all gone (~70 lines), along with the extra pass over the
divisor array that ran ahead of `rem` on every ANSI-mode float remainder.
Two deviations from your sketch:
1. The sketch always returns `ColumnarValue::Array`. DataFusion's `apply`
returns a `Scalar` for scalar/scalar operands (it calls
`ScalarValue::try_from_array(array, 0)` in that arm), and returning a length-1
array when the batch has N rows would be wrong, so I convert back to a scalar
in that case. `test_modulo_both_literals_non_zero_divisor_float64_ansi` pins
that down.
2. I added a `left_data_type == right_data_type` guard on the branch. Spark
casts `Remainder` operands to a common type so this should always hold, but
without it a mismatched pair would panic in `as_primitive::<Float64Type>()`;
with it, the pair falls through to `rem`, which returns an error rather than
silently producing NaN.
I also matched specifically on `ArrowError::DivideByZero` rather than
mapping every error to `remainder_by_zero_error()`, mirroring `checked_binary`
in `checked_arithmetic.rs`, so a length mismatch can't surface as
`REMAINDER_BY_ZERO`.
--
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]
