Add required methods to `NullBufferBuilder` #7013

Chen-Yuan-Lai · 2025-01-24T06:04:08Z

Which issue does this PR close?

Closes #7002 .

Rationale for this change

As #7002 says, some required methods are implemented due to the need to replace BooleanBufferBuilder with NullBufferBuilder in DataFsusion.

What changes are included in this PR?

Implemented methods below:
- capacity()
- is_valid() (as same as get_bit() in BooleanBufferBuilder)
- truncate()
Add related unit tests

Are there any user-facing changes?

…uffer calls

Jefffrey

Thanks for picking this up, left some comments 👍

Jefffrey · 2025-01-24T07:49:19Z

arrow-buffer/src/builder/null.rs

+        if let Some(buf) = self.bitmap_builder.as_ref() {
+            buf.capacity()
+        } else {
+            0


I think need to reference self.capacity here instead of 0?

Actually, I'm not sure when bitmap_builder hasn't been initialized, we should return the actual capacity of it (that is 0 because no builder existed) or self.capacity.

In my opinion it would feel weird if I initialize a NullBufferBuilder with a capacity:

let nbb = NullBufferBuilder::new(10)

But then checking the capacity right after would result in it saying 0.

I agree it is a bit strange, but I think the PR as written makes the most sense.

Specifically, downstream in DataFusion (and other places) we use the capacity as a way to calculate how much memory has been allocated -- if there is no bitmap_builder there is no memory allocated.

I think we can make this less confusing with some comments. I left some suggestions

Another thing that might help might be to rename it to fn allocated_capacity() to make the difference more explicit 🤔

I also made Minor: Clarify NullBufferBuilder::new capacity parameter #7016 to try and clarify the behavior of capacity

fn allocated_capacity() would make sense, but it seems we already have that:

arrow-rs/arrow-buffer/src/builder/null.rs

Lines 188 to 194 in 0c07ec7

/// Return the allocated size of this builder, in bytes, useful for memory accounting.

pub fn allocated_size(&self) -> usize {

self.bitmap_builder

.as_ref()

.map(|b| b.capacity())

.unwrap_or(0)

}

Or rename capacity field to initial_capacity or initial_bits to clearly indicate this is for initialization only?

pub fn new(capacity: usize) -> Self { Self { bitmap_builder: None, len: 0, initial_capacity, } }

arrow-buffer/src/builder/null.rs

Jefffrey · 2025-01-24T07:52:18Z

arrow-buffer/src/builder/null.rs

+        if len > self.len {
+            return;
+        }


Once we materialize, is self.len accurate anymore?

I am not sure what you mean -- in this case the user requested to truncate to a value larger than the current length which has no effect.

This behavior seems to be consistent with Vec::truncate so it makes sense to me: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.truncate

Consider this test case:

#[test] fn test123() { let mut builder = NullBufferBuilder::new(0); assert_eq!(builder.len(), 0); builder.append_n_nulls(2); assert_eq!(builder.len(), 2); builder.truncate(1); assert_eq!(builder.len(), 1); // fails here }

It would fail at the last assertion because it was materialized after appending two nulls, but then truncating down to 1 is a noop since the internal self.len stays 0 (not updated after materialization)

You are (of course) correct. This is a great test and find

I took the liberty of pushing a commit to this branch that includes this test case (and will fail CI until it is fixed so block this PR from merging)

alamb

Thanks @Chen-Yuan-Lai and @Jefffrey -- I think this PR looks really nice.

The comments / suggestions from @Jefffrey are worth looking at I think but in my opinion we could also merge this PR as is.

I will wait for @Jefffrey 's comments before doing so

alamb · 2025-01-24T19:56:57Z

arrow-buffer/src/builder/null.rs

+        if let Some(buf) = self.bitmap_builder.as_ref() {
+            buf.capacity()
+        } else {
+            0


I agree it is a bit strange, but I think the PR as written makes the most sense.

Specifically, downstream in DataFusion (and other places) we use the capacity as a way to calculate how much memory has been allocated -- if there is no bitmap_builder there is no memory allocated.

I think we can make this less confusing with some comments. I left some suggestions

Another thing that might help might be to rename it to fn allocated_capacity() to make the difference more explicit 🤔

alamb · 2025-01-24T19:59:34Z

arrow-buffer/src/builder/null.rs

@@ -134,6 +133,42 @@ impl NullBufferBuilder {
        }
    }

+    /// Returns the capacity of the buffer


Suggested change

/// Returns the capacity of the buffer

/// Returns the allocated capacity of the buffer, if any

///

/// Note: if no nulls have been added, no buffer has been materialized

/// and this function will return 0, regardless of the capacity passed to `Self::new`

alamb · 2025-01-24T20:00:59Z

arrow-buffer/src/builder/null.rs

+        if len > self.len {
+            return;
+        }


I am not sure what you mean -- in this case the user requested to truncate to a value larger than the current length which has no effect.

This behavior seems to be consistent with Vec::truncate so it makes sense to me: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.truncate

Ian Lai and others added 5 commits January 23, 2025 10:06

feat: add required methods to NullBufferBuilder

f652665

modify realated unit tests

0ddb588

remove blank

1d65de7

fix typo & naming

f5b3110

add more assert in unit tests

3c963a5

github-actions bot added the arrow Changes to the arrow crate label Jan 24, 2025

Ian Lai added 2 commits January 24, 2025 06:42

remove the change for sync self.capacity in apped/truncate/new_from_b…

847e5dc

…uffer calls

remove ';'

cf5ec9a

Jefffrey reviewed Jan 24, 2025

View reviewed changes

refactor NullBufferBuilder methods to use references for bitmap_builder

5246cfc

alamb approved these changes Jan 24, 2025

View reviewed changes

alamb mentioned this pull request Jan 24, 2025

Minor: Clarify NullBufferBuilder::new capacity parameter #7016

Merged

Add another test for truncate

2439103

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add required methods to `NullBufferBuilder` #7013

Add required methods to `NullBufferBuilder` #7013

Chen-Yuan-Lai commented Jan 24, 2025 •

edited

Loading

Jefffrey left a comment

Jefffrey Jan 24, 2025

Chen-Yuan-Lai Jan 24, 2025 •

edited

Loading

Jefffrey Jan 24, 2025 •

edited

Loading

alamb Jan 24, 2025

alamb Jan 24, 2025

Jefffrey Jan 24, 2025

Chen-Yuan-Lai Jan 26, 2025

Jefffrey Jan 24, 2025

alamb Jan 24, 2025

Jefffrey Jan 24, 2025 •

edited

Loading

alamb Jan 24, 2025

alamb left a comment

alamb Jan 24, 2025

alamb Jan 24, 2025

alamb Jan 24, 2025

	/// Return the allocated size of this builder, in bytes, useful for memory accounting.
	pub fn allocated_size(&self) -> usize {
	self.bitmap_builder
	.as_ref()
	.map(\|b\| b.capacity())
	.unwrap_or(0)
	}

-    /// Returns the capacity of the buffer
+    /// Returns the allocated capacity of the buffer, if any
+    ///
+    /// Note: if no nulls have been added, no buffer has been materialized
+    /// and this function will return 0, regardless of the capacity passed to `Self::new`

Add required methods to NullBufferBuilder #7013

Are you sure you want to change the base?

Add required methods to NullBufferBuilder #7013

Conversation

Chen-Yuan-Lai commented Jan 24, 2025 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Jefffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chen-Yuan-Lai Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Jefffrey Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jefffrey Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add required methods to `NullBufferBuilder` #7013

Add required methods to `NullBufferBuilder` #7013

Chen-Yuan-Lai commented Jan 24, 2025 •

edited

Loading

Chen-Yuan-Lai Jan 24, 2025 •

edited

Loading

Jefffrey Jan 24, 2025 •

edited

Loading

Jefffrey Jan 24, 2025 •

edited

Loading