sunchao commented on code in PR #1983:
URL: https://github.com/apache/arrow-rs/pull/1983#discussion_r916178915
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -126,44 +126,68 @@ where
let null_bit_buffer =
combine_option_bitmap(&[left.data_ref(), right.data_ref()],
left.len())?;
- let buffer = if let Some(b) = &null_bit_buffer {
- let values = left.values().iter().zip(right.values()).enumerate().map(
- |(i, (left, right))| {
- let is_valid = unsafe { bit_util::get_bit_raw(b.as_ptr(), i) };
- if is_valid {
- if right.is_zero() {
- Err(ArrowError::DivideByZero)
- } else {
- Ok(op(*left, *right))
- }
+ math_checked_divide_op_on_iters(
+ left.into_iter(),
+ right.into_iter(),
+ op,
+ left.len(),
+ null_bit_buffer,
+ )
+}
+
+/// Helper function for operations where a valid `0` on the right array should
+/// result in an [ArrowError::DivideByZero], namely the division and modulo
operations
+///
+/// # Errors
+///
+/// This function errors if:
+/// * the arrays have different lengths
+/// * there is an element where both left and right values are valid and the
right value is `0`
+fn math_checked_divide_op_on_iters<T, F>(
+ left: impl Iterator<Item = Option<T::Native>>,
+ right: impl Iterator<Item = Option<T::Native>>,
+ op: F,
+ len: usize,
+ null_bit_buffer: Option<Buffer>,
+) -> Result<PrimitiveArray<T>>
+where
+ T: ArrowNumericType,
+ T::Native: One + Zero,
+ F: Fn(T::Native, T::Native) -> T::Native,
+{
+ let buffer = if null_bit_buffer.is_some() {
+ let values = left.zip(right).map(|(left, right)| {
+ if let (Some(l), Some(r)) = (left, right) {
+ if r.is_zero() {
+ Err(ArrowError::DivideByZero)
} else {
- Ok(T::default_value())
+ Ok(op(l, r))
}
- },
- );
+ } else {
+ Ok(T::default_value())
Review Comment:
curious why we are using default value here
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -126,44 +126,68 @@ where
let null_bit_buffer =
combine_option_bitmap(&[left.data_ref(), right.data_ref()],
left.len())?;
- let buffer = if let Some(b) = &null_bit_buffer {
- let values = left.values().iter().zip(right.values()).enumerate().map(
- |(i, (left, right))| {
- let is_valid = unsafe { bit_util::get_bit_raw(b.as_ptr(), i) };
- if is_valid {
- if right.is_zero() {
- Err(ArrowError::DivideByZero)
- } else {
- Ok(op(*left, *right))
- }
+ math_checked_divide_op_on_iters(
+ left.into_iter(),
+ right.into_iter(),
+ op,
+ left.len(),
+ null_bit_buffer,
+ )
+}
+
+/// Helper function for operations where a valid `0` on the right array should
+/// result in an [ArrowError::DivideByZero], namely the division and modulo
operations
+///
+/// # Errors
+///
+/// This function errors if:
+/// * the arrays have different lengths
+/// * there is an element where both left and right values are valid and the
right value is `0`
+fn math_checked_divide_op_on_iters<T, F>(
+ left: impl Iterator<Item = Option<T::Native>>,
+ right: impl Iterator<Item = Option<T::Native>>,
+ op: F,
+ len: usize,
+ null_bit_buffer: Option<Buffer>,
Review Comment:
nit: maybe we can just pass a boolean flag here
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -671,6 +741,28 @@ where
math_dict_op!(left, right, op, PrimitiveArray<T>)
}
+/// Helper function for operations where a valid `0` on the right array should
+/// result in an [ArrowError::DivideByZero], namely the division and modulo
operations
+///
+/// # Errors
+///
+/// This function errors if:
+/// * the arrays have different lengths
+/// * there is an element where both left and right values are valid and the
right value is `0`
+fn math_divide_checked_op_dict<K, T, F>(
+ left: &DictionaryArray<K>,
+ right: &DictionaryArray<K>,
+ op: F,
+) -> Result<PrimitiveArray<T>>
+where
+ K: ArrowNumericType,
+ T: ArrowNumericType,
+ T::Native: One + Zero,
+ F: Fn(T::Native, T::Native) -> T::Native,
+{
+ math_dict_divide_checked_op!(left, right, op, PrimitiveArray<T>)
Review Comment:
why we need a macro for this? can we just inline it?
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -892,6 +984,18 @@ where
return math_checked_divide_op(left, right, |a, b| a / b);
}
+/// Perform `left / right` operation on two arrays. If either left or right
value is null
+/// then the result is also null. If any right hand value is zero then the
result of this
+/// operation will be `Err(ArrowError::DivideByZero)`.
+pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
+ match left.data_type() {
Review Comment:
what if right is not a dictionary? are we going to support it too?
--
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]
