-
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-45820: [C++] Add optional out_offset for Buffer-returning CopyBitmap function #45852
Conversation
…pyBitmap function
cpp/src/arrow/util/bit_util_test.cc
Outdated
for (bool maintan_offset : {true, false}) { | ||
for (int64_t num_bits : lengths) { | ||
for (int64_t offset : offsets) { | ||
const int64_t copy_length = num_bits - offset; |
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.
It would be better to test with different offsets, for example:
for (bool maintan_offset : {true, false}) { | |
for (int64_t num_bits : lengths) { | |
for (int64_t offset : offsets) { | |
const int64_t copy_length = num_bits - offset; | |
for (int64_t num_bits : lengths) { | |
for (int64_t offset : offsets) { | |
for (int64_t dest_offset : {0, offset - 1, offset + 1}) { | |
const int64_t copy_length = num_bits - offset; |
cpp/src/arrow/util/bitmap_ops.cc
Outdated
// As we have freshly allocated this bitmap, we should take care of zeroing the | ||
// remaining bits. |
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.
It is actually dubious that this code is necessary, since AllocateEmptyBitmap
has already zero-initialized the entire buffer. Perhaps we can try remove it?
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.
I've tried removing it and all tests are successful
@github-actions crossbow submit -g cpp |
Revision: c6e8149 Submitted crossbow builds: ursacomputing/crossbow @ actions-6ed5547d68 |
Valgrind tests have passed, which is a confirmation that the removed code did not serve a purpose. |
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.
Thanks for this @raulcd ! CI failures are unrelated.
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 5d487ca. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 19 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
The
CopyBitmap
variant that returns astd::shared_ptr<Buffer>
doesn't allow passing a destination bit offset for the output.Accepting such an argument would allow using that function to replace the null bitmap of an existing array, without changing its offset.
What changes are included in this PR?
Adding the option
out_offset
argument.Are these changes tested?
Yes, the existing
TestCopyBitmap
has been adapted to usedest_offset
or not.Are there any user-facing changes?
No, but we allow a new optional argument (
out_offset
) toCopyBitmap
CopyBitmap
should take an optional out offset. #45820