[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #1781: Arbitrary size combine option bitmap

[GitBox <gi...@apache.org>]

HaoYang670 commented on code in PR #1781:
URL: https://github.com/apache/arrow-rs/pull/1781#discussion_r889526712


##########
arrow/src/compute/util.rs:
##########
@@ -29,33 +29,42 @@ use std::ops::Add;
 /// This function is useful when implementing operations on higher level arrays.
 #[allow(clippy::unnecessary_wraps)]
 pub(super) fn combine_option_bitmap(
-    left_data: &ArrayData,
-    right_data: &ArrayData,
+    arrays: &[&ArrayData],
     len_in_bits: usize,
 ) -> Result<Option<Buffer>> {
-    let left_offset_in_bits = left_data.offset();
-    let right_offset_in_bits = right_data.offset();
-
-    let left = left_data.null_buffer();
-    let right = right_data.null_buffer();
-
-    match left {
-        None => match right {
-            None => Ok(None),
-            Some(r) => Ok(Some(r.bit_slice(right_offset_in_bits, len_in_bits))),
-        },
-        Some(l) => match right {
-            None => Ok(Some(l.bit_slice(left_offset_in_bits, len_in_bits))),
-
-            Some(r) => Ok(Some(buffer_bin_and(
-                l,
-                left_offset_in_bits,
-                r,
-                right_offset_in_bits,
-                len_in_bits,
-            ))),
-        },
+    if arrays.is_empty() {
+        return Err(ArrowError::ComputeError(
+            "Arrays must not be empty".to_string(),
+        ));
     }
+
+    let mut buffers = arrays
+        .iter()
+        .map(|array| (array.null_buffer(), array.offset()));
+
+    let init = buffers.next().unwrap();
+    Ok(buffers
+        .fold((init.0.cloned(), init.1), |acc, buffer| {

Review Comment:
   I am not sure but it seems like `reduce` is more suitable than `fold` in this context.
   So maybe we could rewrite like:
   ```rust
   arrays
       .iter()
       .map(|arr| {(arr.null_buffer(), arr.offset()})
       .reduce(|(buf1, offset1), (buf2, offset2)| {
           ...
       })
   ```



##########
arrow/src/compute/util.rs:
##########
@@ -29,33 +29,42 @@ use std::ops::Add;
 /// This function is useful when implementing operations on higher level arrays.
 #[allow(clippy::unnecessary_wraps)]
 pub(super) fn combine_option_bitmap(
-    left_data: &ArrayData,
-    right_data: &ArrayData,
+    arrays: &[&ArrayData],
     len_in_bits: usize,
 ) -> Result<Option<Buffer>> {
-    let left_offset_in_bits = left_data.offset();
-    let right_offset_in_bits = right_data.offset();
-
-    let left = left_data.null_buffer();
-    let right = right_data.null_buffer();
-
-    match left {
-        None => match right {
-            None => Ok(None),
-            Some(r) => Ok(Some(r.bit_slice(right_offset_in_bits, len_in_bits))),
-        },
-        Some(l) => match right {
-            None => Ok(Some(l.bit_slice(left_offset_in_bits, len_in_bits))),
-
-            Some(r) => Ok(Some(buffer_bin_and(
-                l,
-                left_offset_in_bits,
-                r,
-                right_offset_in_bits,
-                len_in_bits,
-            ))),
-        },
+    if arrays.is_empty() {
+        return Err(ArrowError::ComputeError(
+            "Arrays must not be empty".to_string(),
+        ));
     }
+
+    let mut buffers = arrays

Review Comment:
   Maybe we should rename it as `buffer_and_offset` or something like that. 



##########
arrow/src/compute/util.rs:
##########
@@ -29,33 +29,42 @@ use std::ops::Add;
 /// This function is useful when implementing operations on higher level arrays.

Review Comment:
   Nit: we should also update the docs



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org