[GitHub] [arrow-rs] alamb commented on a change in pull request #1232: Fix null bitmap length validation (#1231)

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

alamb commented on a change in pull request #1232:
URL: https://github.com/apache/arrow-rs/pull/1232#discussion_r791071959



##########
File path: arrow/src/array/data.rs
##########
@@ -1621,14 +1622,14 @@ mod tests {
     }
 
     #[test]
-    #[should_panic(expected = "null_bit_buffer size too small. got 8 needed 13")]
+    #[should_panic(expected = "null_bit_buffer size too small. got 1 needed 2")]
     fn test_bitmap_too_small() {
-        let buffer = make_i32_buffer(100);
+        let buffer = make_i32_buffer(9);

Review comment:
       I don't understand why changes (other than the message) are needed for this test. 
   
   The original formulation with 100 still fails even after this PR, but with a different error message

##########
File path: arrow/src/array/data.rs
##########
@@ -650,7 +650,8 @@ impl ArrayData {
         }
 
         // check null bit buffer size
-        if let Some(null_bit_buffer) = self.null_bitmap.as_ref() {
+        if let Some(null_bit_map) = self.null_bitmap.as_ref() {
+            let null_bit_buffer = null_bit_map.buffer_ref();

Review comment:
       I don't understand this change -- the length of the `BitMap` is in `bits`  which seems like it is properly handled in `bit_util::ceil()` on the next linel
   
   If you change this to use the underlying buffer, then the length is in bytes, so then the division should also be removed?

##########
File path: arrow/src/array/data.rs
##########
@@ -1621,14 +1622,14 @@ mod tests {
     }
 
     #[test]
-    #[should_panic(expected = "null_bit_buffer size too small. got 8 needed 13")]
+    #[should_panic(expected = "null_bit_buffer size too small. got 1 needed 2")]
     fn test_bitmap_too_small() {
-        let buffer = make_i32_buffer(100);
+        let buffer = make_i32_buffer(9);

Review comment:
       I am confused -- if the size calculation really was off, I would expect this code to fail ... But it passes just fine on master without the changes  in this PR. So 🤔 
   
   ```
       #[test]
       fn test_bitmap_size_ok_ok() {
           let buffer = make_i32_buffer(8);
           let null_bit_buffer = Buffer::from(vec![0b11111111]);
   
           ArrayData::try_new(
               DataType::Int32,
               8,
               Some(0),
               Some(null_bit_buffer),
               0,
               vec![buffer],
               vec![],
           ).unwrap();
       }
   ```




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