-
Notifications
You must be signed in to change notification settings - Fork 867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minor: Clarify documentation on NullBufferBuilder::allocated_size
#7089
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,7 +83,24 @@ impl BooleanBufferBuilder { | |
self.len == 0 | ||
} | ||
|
||
/// Returns the capacity of the buffer | ||
/// Returns the capacity of the buffer, in bits (not bytes) | ||
/// | ||
/// Note this | ||
/// | ||
/// # Example | ||
/// ``` | ||
/// # use arrow_buffer::builder::BooleanBufferBuilder; | ||
/// // empty requires 0 bytes | ||
/// let b = BooleanBufferBuilder::new(0); | ||
/// assert_eq!(0, b.capacity()); | ||
/// // Creating space for 1 bit results in 64 bytes (space for 512 bits) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a small note here explaining why it over allocates? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 12055a0 |
||
/// // (64 is the minimum allocation size for 64 bit architectures) | ||
/// let mut b = BooleanBufferBuilder::new(1); | ||
/// assert_eq!(512, b.capacity()); | ||
/// // 1000 bits requires 128 bytes (space for 1024 bits) | ||
/// b.append_n(1000, true); | ||
/// assert_eq!(1024, b.capacity()); | ||
/// ``` | ||
#[inline] | ||
pub fn capacity(&self) -> usize { | ||
self.buffer.capacity() * 8 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -217,7 +217,7 @@ impl NullBufferBuilder { | |
self.bitmap_builder.as_mut().map(|b| b.as_slice_mut()) | ||
} | ||
|
||
/// Return the allocated size of this builder, in bytes, useful for memory accounting. | ||
/// Return the allocated size of this builder, in bits, useful for memory accounting. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This documentation is wrong -- as it uses I actually think it would be better if we had a function that returned allocated size in bytes (maybe we can deprecate this one and make a new one like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this is a bug and we could fix this without it being a breaking change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created #7121 |
||
pub fn allocated_size(&self) -> usize { | ||
self.bitmap_builder | ||
.as_ref() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key detail is that this function returns the value in
bits
(not bytes)