Add `NullBuffer::from_unsliced_buffer` helper and refactor call sites by Eyad3skr · Pull Request #9411 · apache/arrow-rs

Eyad3skr · 2026-02-13T18:38:13Z

Implements a helper to replace the pattern of creating a BooleanBuffer from an unsliced validity bitmap and filtering by null count. Previously this was done with BooleanBuffer::new(...) plus Some(NullBuffer::new(...)).filter(|n| n.null_count() > 0); now it is a single call to NullBuffer::try_from_unsliced(buffer, len), which returns Some(NullBuffer) when there are nulls and None when all values are valid.

Added try_from_unsliced in arrow-buffer/src/buffer/null.rs with tests for nulls, all valid, all null, empty
Refactor FixedSizeBinaryArray::try_from_iter_with_size and try_from_sparse_iter_with_size to use it
Refactor take_nulls in arrow-select to use it

Closes #9385

alamb

Thank you @Eyad3skr -- this looks nice to me

cc @liamzwbao in case you have time to help reivew

alamb · 2026-02-13T22:00:50Z

arrow-buffer/src/buffer/null.rs

    pub fn buffer(&self) -> &Buffer {
        self.buffer.inner()
    }
+    /// Create a [`NullBuffer`] from an *unsliced* validity bitmap (`offset = 0`) of length `len`.


Could you also update this comment to reflect the offset is in bits (not bytes)? Mixing the units is a common mistake so making sure the documentation is as clear as possible would help

Jefffrey · 2026-02-14T03:19:33Z

arrow-buffer/src/buffer/null.rs

+    /// Create a [`NullBuffer`] from an *unsliced* validity bitmap (`offset = 0`) of length `len`.
+    ///
+    /// Returns `None` if there are no nulls (all values valid).
+    pub fn try_from_unsliced(buffer: impl Into<Buffer>, len: usize) -> Option<Self> {


Maybe we should call it from_buffer? try_* probably should be reserved for functions that return a result, and its probably better to be direct that we're constructing directly from a buffer?

a valid point tbh! on it.

Eyad3skr · 2026-02-15T19:26:37Z

@alamb I guess there is nothing else to take care after anymore? maybe if someone can just trigger/approve the CI pipeline workflows left that would be awesome.

alamb · 2026-02-16T11:25:55Z

@alamb I guess there is nothing else to take care after anymore? maybe if someone can just trigger/approve the CI pipeline workflows left that would be awesome.

Done!

alamb · 2026-02-17T13:04:39Z

Seems like the CI is broken

Eyad3skr · 2026-02-18T16:50:40Z

true, my schedule this week is a bit busy because of university. I will be working on fixing the CI between today and Friday. On it.

liamzwbao

Overall LGTM! thanks for the change.

It would be better to refactor the following call sites as well

arrow-rs/arrow-array/src/array/boolean_array.rs

Lines 537 to 542 in c129c7c

    
           let nulls = Some(NullBuffer::new(BooleanBuffer::new( 
        
               null_builder.into(), 
        
               0, 
        
               data_len, 
        
           ))) 
        
           .filter(|n| n.null_count() > 0);

arrow-rs/arrow-string/src/regexp.rs

Lines 203 to 207 in c129c7c

    
           let nulls = array 
        
               .nulls() 
        
               .map(|n| n.inner().sliced()) 
        
               .map(|b| NullBuffer::new(BooleanBuffer::new(b, 0, array.len()))) 
        
               .filter(|n| n.null_count() > 0);

arrow-rs/arrow-string/src/substring.rs

Lines 216 to 220 in c129c7c

    
           let nulls = array 
        
               .nulls() 
        
               .map(|n| n.inner().sliced()) 
        
               .map(|b| NullBuffer::new(BooleanBuffer::new(b, 0, array.len()))) 
        
               .filter(|n| n.null_count() > 0);

arrow-rs/arrow-string/src/substring.rs

Lines 318 to 322 in c129c7c

    
           let nulls = array 
        
               .nulls() 
        
               .map(|n| n.inner().sliced()) 
        
               .map(|b| NullBuffer::new(BooleanBuffer::new(b, 0, array.len()))) 
        
               .filter(|n| n.null_count() > 0);

arrow-rs/arrow-string/src/substring.rs

Lines 356 to 360 in c129c7c

    
           let mut nulls = array 
        
               .nulls() 
        
               .map(|n| n.inner().sliced()) 
        
               .map(|b| NullBuffer::new(BooleanBuffer::new(b, 0, num_of_elements))) 
        
               .filter(|n| n.null_count() > 0);

liamzwbao · 2026-02-19T00:59:35Z

arrow-buffer/src/buffer/null.rs

+    #[test]
+    fn test_from_unsliced_buffer_with_nulls() {
+        // Buffer with some nulls: 0b10110010 = valid, null, valid, valid, null, null, valid, null
+        let buf = Buffer::from([0b10110010u8]);


Arrow uses LSB numbering and that's why this test failed. Could refer to the doc to fix the test

Oh!! Thanks for the info. going to check it now!

…op unused BooleanBuffer import in parquet view_buffer.rs

Eyad3skr · 2026-02-20T19:48:48Z

would appreciate if you can trigger/approve on CI workflows @alamb

liamzwbao · 2026-02-20T19:43:04Z

arrow-string/src/regexp.rs

+    let null_bit_buffer = array
+        .nulls()
+        .map(|n| n.inner().sliced())
+        .and_then(|b| NullBuffer::from_unsliced_buffer(b, array.len()))
+        .map(|nb| nb.into_inner().into_inner());


We don't need a new null_bit_buffer, let's refactor line 208 to 212 at the end of this func instead

liamzwbao · 2026-02-20T19:48:35Z

arrow-string/src/substring.rs

+    // FixedSizeBinaryArray with size 0 requires a validity bitmap
    if new_len == 0 && nulls.is_none() {
-        // FixedSizeBinaryArray::new takes length from the values buffer, except when size == 0.
-        // In that case it uses the null buffer length, so preserve the original length here.
-        // Example: ["", "", ""] -> substring(..., 1, Some(2)) should keep len=3;
-        // otherwise it collapses to an empty array (len=0).


We should keep the existing comment here and avoid adding the new comment on line 360, as it is unrelated to this PR.

refinement for comments and null_bit_buffer

liamzwbao · 2026-02-20T20:03:04Z

arrow-string/src/substring.rs

+        .and_then(|b| NullBuffer::from_unsliced_buffer(b, num_of_elements));
+
+    // FixedSizeBinaryArray::new takes length from the values buffer, except when size == 0.
+    // In that case it uses the null buffer length, so preserve the original length here.
+    // Example: ["", "", ""] -> substring(..., 1, Some(2)) should keep len=3;
+    // otherwise it collapses to an empty array (len=0).
    if new_len == 0 && nulls.is_none() {
-        // FixedSizeBinaryArray::new takes length from the values buffer, except when size == 0.
-        // In that case it uses the null buffer length, so preserve the original length here.
-        // Example: ["", "", ""] -> substring(..., 1, Some(2)) should keep len=3;
-        // otherwise it collapses to an empty array (len=0).


nit: can we move the comment to its original place?

fair enough

liamzwbao

LGTM! But I guess CI will fail due to formatting/import issues.

You could run cargo fmt and cargo check to check it out

…ed imports

Eyad3skr · 2026-02-20T20:30:04Z

LGTM! But I guess CI will fail due to formatting/import issues.

You could run cargo fmt and cargo check to check it out

true, I forgot about 2 warning from unused imports.. thanks!

Eyad3skr · 2026-02-21T23:20:07Z

would appreciate if you can trigger the CI workflows for me @alamb

alamb

This looks great -- thanks a lot @Eyad3skr and @liamzwbao and @Jefffrey . A good team effort

Add NullBuffer::try_from_unsliced helper and refactor call sites

d9cc893

github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels Feb 13, 2026

alamb approved these changes Feb 13, 2026

View reviewed changes

Jefffrey reviewed Feb 14, 2026

View reviewed changes

function name refactoring and adjusting comments

7aa4125

liamzwbao reviewed Feb 19, 2026

View reviewed changes

Eyad3skr and others added 6 commits February 20, 2026 15:39

Correct LSB bit ordering in test_from_unsliced_buffer_with_nulls & Dr…

95ae1db

…op unused BooleanBuffer import in parquet view_buffer.rs

Correct LSB bit ordering in test_from_unsliced_buffer_with_nulls & Dr…

4ab1c75

…op unused BooleanBuffer import in parquet view_buffer.rs

merge conflict resolving apache#1

195f5bc

merge conflict resolving apache#2

060402a

merge conflict resolving apache#3 and running cargo fmt --all

3b49bdb

Merge branch 'main' into feat/null-buffer-try-from-unsliced

f39ad6f

liamzwbao reviewed Feb 20, 2026

View reviewed changes

refinement

67f8d61

refinement for comments and null_bit_buffer

liamzwbao reviewed Feb 20, 2026

View reviewed changes

moving new_len comment in substring.rs to it's original place

7acbdb8

liamzwbao approved these changes Feb 20, 2026

View reviewed changes

running cargo fmt --all and cargo check, removed 2 warnings from unus…

1c82f1d

…ed imports

Jefffrey changed the title ~~Add NullBuffer::try_from_unsliced helper and refactor call sites~~ Add NullBuffer::from_unsliced_buffer helper and refactor call sites Feb 21, 2026

Jefffrey approved these changes Feb 21, 2026

View reviewed changes

fixing CI errors by importing in test module in substring.rs

7112516

alamb approved these changes Feb 24, 2026

View reviewed changes

alamb merged commit a2cffdb into apache:main Feb 24, 2026
28 checks passed

	let nulls = Some(NullBuffer::new(BooleanBuffer::new(
	null_builder.into(),
	0,
	data_len,
	)))
	.filter(\|n\| n.null_count() > 0);

	let nulls = array
	.nulls()
	.map(\|n\| n.inner().sliced())
	.map(\|b\| NullBuffer::new(BooleanBuffer::new(b, 0, array.len())))
	.filter(\|n\| n.null_count() > 0);

	let mut nulls = array
	.nulls()
	.map(\|n\| n.inner().sliced())
	.map(\|b\| NullBuffer::new(BooleanBuffer::new(b, 0, num_of_elements)))
	.filter(\|n\| n.null_count() > 0);

Conversation

Eyad3skr commented Feb 13, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eyad3skr commented Feb 15, 2026

Uh oh!

alamb commented Feb 16, 2026

Uh oh!

alamb commented Feb 17, 2026

Uh oh!

Eyad3skr commented Feb 18, 2026

Uh oh!

liamzwbao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eyad3skr commented Feb 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamzwbao left a comment

Choose a reason for hiding this comment

Uh oh!

Eyad3skr commented Feb 20, 2026

Uh oh!

Eyad3skr commented Feb 21, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants