[GitHub] [arrow-rs] alamb commented on a diff in pull request #3885: Flesh out NullBuffer abstraction (#3880)

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #3885:
URL: https://github.com/apache/arrow-rs/pull/3885#discussion_r1142594006


##########
arrow-arith/src/arity.rs:
##########
@@ -412,14 +388,8 @@ where
             Err(err) => return Ok(Err(err)),
         };
 
-        let array_builder = builder
-            .finish()
-            .into_data()
-            .into_builder()
-            .null_bit_buffer(null_buffer)
-            .null_count(null_count);
-
-        let array_data = unsafe { array_builder.build_unchecked() };
+        let array_builder = builder.finish().into_data().into_builder();

Review Comment:
   I wonder why we need to make a new builder (rather than just update the one we had) -- though I see this is what the previous code did too



##########
arrow-buffer/src/buffer/boolean.rs:
##########
@@ -145,3 +146,41 @@ impl BooleanBuffer {
         self.buffer
     }
 }
+
+impl BitAnd<&BooleanBuffer> for &BooleanBuffer {

Review Comment:
   This is very nice 👍  -- it will be really nice to apply such bit operations on BooleanBuffer / NullBuffers directly and not via some function
   



##########
arrow-arith/src/arity.rs:
##########
@@ -391,17 +372,12 @@ where
     if a.null_count() == 0 && b.null_count() == 0 {
         try_binary_no_nulls_mut(len, a, b, op)
     } else {
-        let null_buffer = combine_option_bitmap(&[a.data(), b.data()], len);
-        let null_count = null_buffer

Review Comment:
   it is nice that the null count now is encapsulated in `NullBuffer` rather than needing to be calculated explicitly (and potentially inconsistently)



##########
arrow-buffer/src/buffer/boolean.rs:
##########
@@ -145,3 +146,41 @@ impl BooleanBuffer {
         self.buffer
     }
 }
+
+impl BitAnd<&BooleanBuffer> for &BooleanBuffer {
+    type Output = BooleanBuffer;
+
+    fn bitand(self, rhs: &BooleanBuffer) -> Self::Output {
+        assert_eq!(self.len, rhs.len);
+        BooleanBuffer {
+            buffer: buffer_bin_and(

Review Comment:
   I noticed above that buffers are is combined using `combine_option_bitmap`  which is different than this. 
   
   However, when I double checked that `combine_option_bitmap` calls eventually to `buffer_bin_and`
   
   https://github.com/apache/arrow-rs/blob/e41e0ca82ce4967a66a7eda04cb1630487aaeca9/arrow-data/src/bit_mask.rs#L71-L98



-- 
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