alamb commented on code in PR #26062:
URL: https://github.com/apache/datafusion/pull/26062#discussion_r4190149753
##########
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`
Review Comment:
I feel like the mention down here about vectorization is "burying the lead"
so to speak -- it might be better if the comment started with a mention of
"faster version of try_unary that gives the compiler the best chance to
vectorize the check and the operation" or something
##########
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:
though in theory this can run and slow down arrays with nulls
##########
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>(
Review Comment:
this seems like it might be a good one to propose porting upstream to arrows
rs (or as an example on `try_unary` 🤔
##########
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();
Review Comment:
it is interesting this is mre vectorizable -- I was sot of expecting two
loops
##########
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:
👍
--
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]
