-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
GH-45819: [C++] Add OptionalBitmapAnd utility function #45869
base: main
Are you sure you want to change the base?
Conversation
@github-actions crossbow submit -g cpp |
Revision: 4188841 Submitted crossbow builds: ursacomputing/crossbow @ actions-fd8ca4245c |
CI failures are unrelated. Happy to rebase once they are fixed. @pitrou is something like this what you had in mind? |
/// their respective bit-offsets for the given bit-length and put | ||
/// the results in out_buffer starting at the given bit-offset. | ||
/// Both right and left buffers are optional. If any of the buffers is | ||
/// null a bitmap of zeros is returned. |
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.
Hmm, actually, the more useful semantics is that a null pointer means the bitmap is all-1s.
This reflects the situation where an Array doesn't have a null bitmap: all values are valid.
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.
And this would be even more useful as:
Result<std::shared_ptr<Buffer>> OptionalBitmapAnd(
MemoryPool* pool, const std::shared_ptr<Buffer>& left, int64_t left_offset,
const std::shared_ptr<Buffer>& right, int64_t right_offset, int64_t out_offset);
... because then, if one of the inputs is null, and the offsets are compatible, we can return the other input (perhaps sliced) instead of allocating a new buffer.
Rationale for this change
Bitmaps are optional, having the possibility an utility function that allows us to do a
BitmapAnd
operation that takes into account the possibility of a bitmap being null can be handy as we don't have to cater for the specific case.What changes are included in this PR?
Add a new
OptionalBitmapAnd
function.Are these changes tested?
Yes.
Are there any user-facing changes?
No, just a new utility function.
OptionalBitmapAnd
utility #45819