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:

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

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

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

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