Skip to content

Conversation

@rluvaton
Copy link
Member

Which issue does this PR close?

N/A

Rationale for this change

Allow for better performance when concatenating null buffer

What changes are included in this PR?

Added append_buffer function in the NullBufferBuilder that just call the underlying BooleanBufferBuilder

Are there any user-facing changes?

yes, new API

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rluvaton

if buffer.null_count() > 0 {
self.materialize_if_needed();
}
if let Some(buf) = self.bitmap_builder.as_mut() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are no nulls in self (so not materalized) and a buffer with nulls is pushed, won't this code ignore those nulls 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as it's the same code as the slice

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be covered by the line above that calls materialize_if_needed

if buffer.null_count() > 0 {
self.materialize_if_needed();
}
if let Some(buf) = self.bitmap_builder.as_mut() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be covered by the line above that calls materialize_if_needed

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rluvaton and @tustvold

@alamb alamb merged commit 8dad535 into apache:main Mar 19, 2025
26 checks passed
@rluvaton rluvaton deleted the add-append-buffer branch March 19, 2025 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow Changes to the arrow crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants