neilconway commented on code in PR #26062:
URL: https://github.com/apache/datafusion/pull/26062#discussion_r4190385390
##########
datafusion/functions/src/math/common.rs:
##########
@@ -150,10 +151,48 @@ pub(crate) fn lcm_signed_int(x: i64, y: i64) ->
Result<i64, ArrowError> {
})
}
+/// Applies `op` to every value in `array`, like `unary`, but returns an error
+/// if `input_error` returns a message for any non-null value.
+///
+/// Use this for functions that return an error for some argument values, such
+/// as `sqrt`, which returns an error for negative numbers. `try_unary` can
also
+/// return errors, but it can return early on any value, which keeps the
+/// compiler from vectorizing its loop. That makes cheap functions like `sqrt`
+/// several times slower.
+///
+/// Instead, `input_error` is called on every value, including those in null
+/// slots, in the same loop as `op`; only if some value fails is the array
+/// searched again for an error to report. `input_error` should therefore be a
+/// cheap check, such as a comparison.
+pub(crate) fn unary_with_input_check<T: ArrowPrimitiveType>(
+ array: &PrimitiveArray<T>,
+ op: impl Fn(T::Native) -> T::Native,
+ input_error: impl Fn(T::Native) -> Option<&'static str>,
+) -> Result<PrimitiveArray<T>> {
+ let mut any_invalid = false;
+ let values: Vec<T::Native> = array
+ .values()
+ .iter()
+ .map(|&x| {
+ any_invalid |= input_error(x).is_some();
+ op(x)
+ })
+ .collect();
+
+ // The check above also ran on null slots, which can hold any value, so the
+ // failure may be spurious. Re-check just the non-null values.
Review Comment:
Ah, right, this is a very good point! `try_unary` on NULL-heavy inputs can
actually be faster. Measuring on my local machine:
* For cheap functions like `sqrt`, `degrees`, `radians`, `unary` is faster
for <= 80% NULLs.
* For expensive functions like `exp`, `sin`, and `ln`, the cross-over point
is something like 25% NULLs.
In an extreme case of 90% nulls (randomly distributed), `exp` is about 5
times faster using `try_unary` than with `unary`. Whereas with no nulls,
`unary` is about 10% faster.
Intuitively, expensive functions (a) can't be vectorized anyway (b) waste
more work on the values in null slots.
I think reverting the blanket switch to `try_unary` stlll makes sense,
because it was probably unintended, and I'd suspect that most people invoking
math functions on large data sets won't have NULL-heavy data. But we could
certainly try to make this more intelligent, e.g., by applying a heuristic
based on NULL density to switch to `try_unary`, or by having the more expensive
functions always use `try_unary`. Either way I'd be inclined to leave it to a
followup PR.
--
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]
