Add `is_valid` and `truncate` 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, } }

I think we don't need to add a capacity() method given allocated_size() seems to already do what is required

Removed capacity method in 8ed7851

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)

Fixed in d0d3c62 and added some more documentation on the relationship between len and builder

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 🤔

arrow-buffer/src/builder/null.rs

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

alamb · 2025-01-27T11:45:31Z

As I am working on the next release, I am going to try and push this PR along

Release arrow-rs / parquet minor version 54.1.0 (Jan 2025) #6929

…-rs into add_required_methods

alamb · 2025-01-27T11:59:24Z

arrow-buffer/src/builder/null.rs

@@ -221,6 +250,7 @@ mod tests {
        builder.append_n_nulls(2);
        builder.append_n_non_nulls(2);
        assert_eq!(6, builder.len());
+        assert_eq!(512, builder.allocated_size());


it appears that @XiangpengHao had already added allocated_size

Since we removed capacity(), I think assertions for builder.allocated_size() can also be removed in the test_null_buffer_builder_truncate test.

Since there was no other testing of allocated_size that I could find, I thought it would be good to leave these tests in as the increase coverage for the existing behavior

alamb · 2025-01-27T11:59:54Z

@Chen-Yuan-Lai and @Jefffrey -- I made some changes to this PR that I think address the (excellent) comments. Let me know what you think

Jefffrey

LGTM 👍

Chen-Yuan-Lai · 2025-01-27T12:24:20Z

LGTM, thanks @alamb and @Jefffrey so much!

alamb · 2025-01-27T12:25:17Z

Thanks again @Chen-Yuan-Lai and @Jefffrey

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

alamb added 5 commits January 27, 2025 06:45

Merge remote-tracking branch 'apache/main' into add_required_methods

290e8ef

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

e98d831

Remove capacity and use allocated_size instead

8ed7851

Merge branch 'add_required_methods' of github.com:Chen-Yuan-Lai/arrow…

174f82c

…-rs into add_required_methods

fix truncate

d0d3c62

alamb changed the title ~~Add required methods to NullBufferBuilder~~ Add is_valid and truncate methods to NullBufferBuilder Jan 27, 2025

alamb reviewed Jan 27, 2025

View reviewed changes

Jefffrey approved these changes Jan 27, 2025

View reviewed changes

alamb mentioned this pull request Jan 27, 2025

Release arrow-rs / parquet minor version 54.1.0 (Jan 2025) #6929

Closed

5 tasks

alamb merged commit 6aaff7e into apache:main Jan 27, 2025
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `is_valid` and `truncate` methods to `NullBufferBuilder` #7013

Add `is_valid` and `truncate` methods to `NullBufferBuilder` #7013

Chen-Yuan-Lai commented Jan 24, 2025 •

edited by alamb

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 26, 2025

alamb Jan 27, 2025

Jefffrey Jan 24, 2025

alamb Jan 24, 2025

Jefffrey Jan 24, 2025 •

edited

Loading

alamb Jan 24, 2025

alamb Jan 27, 2025

alamb left a comment

alamb Jan 24, 2025

alamb Jan 24, 2025

alamb commented Jan 27, 2025

alamb Jan 27, 2025

Chen-Yuan-Lai Jan 27, 2025 •

edited

Loading

alamb Jan 27, 2025

alamb commented Jan 27, 2025

Jefffrey left a comment

Chen-Yuan-Lai commented Jan 27, 2025 •

edited

Loading

alamb commented Jan 27, 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)
	}

Add is_valid and truncate methods to NullBufferBuilder #7013

Add is_valid and truncate methods to NullBufferBuilder #7013

Conversation

Chen-Yuan-Lai commented Jan 24, 2025 • edited by alamb 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

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

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

alamb commented Jan 27, 2025

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jan 27, 2025

Jefffrey left a comment

Choose a reason for hiding this comment

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

alamb commented Jan 27, 2025

Add `is_valid` and `truncate` methods to `NullBufferBuilder` #7013

Add `is_valid` and `truncate` methods to `NullBufferBuilder` #7013

Chen-Yuan-Lai commented Jan 24, 2025 •

edited by alamb

Loading

Chen-Yuan-Lai Jan 24, 2025 •

edited

Loading

Jefffrey Jan 24, 2025 •

edited

Loading

Jefffrey Jan 24, 2025 •

edited

Loading

Chen-Yuan-Lai Jan 27, 2025 •

edited

Loading

Chen-Yuan-Lai commented Jan 27, 2025 •

edited

Loading