Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
pitrou commented on code in PR #45869:
URL: https://github.com/apache/arrow/pull/45869#discussion_r2154138772
##
cpp/src/arrow/util/bitmap_ops.cc:
##
@@ -394,6 +394,31 @@ Result> BitmapOp(MemoryPool* pool,
const uint8_t* left,
} // namespace
+Result> OptionalBitmapAnd(MemoryPool* pool,
+ const
std::shared_ptr& left,
+ int64_t left_offset,
+ const
std::shared_ptr& right,
+ int64_t right_offset,
int64_t length,
+ int64_t out_offset) {
+ if (left == nullptr && right == nullptr) {
+return nullptr;
+ } else if (left == nullptr) {
+if (right_offset == out_offset) {
+ return right;
+}
Review Comment:
It could be a dedicated function `ViewOrCopyBitmap` by the way. Search
through the source code would probably find other places where such a function
could be useful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
pitrou commented on code in PR #45869:
URL: https://github.com/apache/arrow/pull/45869#discussion_r2154134575
##
cpp/src/arrow/util/bitmap_ops.cc:
##
@@ -394,6 +394,31 @@ Result> BitmapOp(MemoryPool* pool,
const uint8_t* left,
} // namespace
+Result> OptionalBitmapAnd(MemoryPool* pool,
+ const
std::shared_ptr& left,
+ int64_t left_offset,
+ const
std::shared_ptr& right,
+ int64_t right_offset,
int64_t length,
+ int64_t out_offset) {
+ if (left == nullptr && right == nullptr) {
+return nullptr;
+ } else if (left == nullptr) {
+if (right_offset == out_offset) {
+ return right;
+}
Review Comment:
We could easily slice the input buffer if both offsets are equal modulo 8.
It would avoid copying in slightly more cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
pitrou commented on code in PR #45869:
URL: https://github.com/apache/arrow/pull/45869#discussion_r2154132901
##
cpp/src/arrow/util/bit_util_test.cc:
##
@@ -1342,25 +1369,48 @@ class BitmapOp : public ::testing::Test {
const std::vector& right_bits,
const std::vector& result_bits) {
std::shared_ptr left, right, out;
-int64_t length;
+int64_t length{0};
+uint8_t *left_data, *right_data;
for (int64_t left_offset : {0, 1, 3, 5, 7, 8, 13, 21, 38, 75, 120, 65536})
{
- BitmapFromVector(left_bits, left_offset, &left, &length);
+ if (left_bits.size() > 0) {
Review Comment:
I think this is putting too much logic in these test methods. I would
suggest something else:
1) test `OptionalBitmapAnd` with both non-null arguments like you do here
2) have separate tests for when one or both of the arguments is null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
raulcd commented on PR #45869: URL: https://github.com/apache/arrow/pull/45869#issuecomment-2980156205 @pitrou this is currently working with the proposed API. I had to do several changes on the tests in order to adapt the existing `TestAligned` and `TestUnaligned` functions. I will appreciate any tip on improving those changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
raulcd commented on PR #45869: URL: https://github.com/apache/arrow/pull/45869#issuecomment-2977017284 I re-started to try and adapt with the new APIs but I am finding adapting the existing tests for the new APIs slightly challenging, I will give that a try again but it is taking me quite a long time, this is the diff for my new branch: https://github.com/apache/arrow/compare/main...raulcd:arrow:GH-45819-2 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
pitrou commented on PR #45869: URL: https://github.com/apache/arrow/pull/45869#issuecomment-2966523239 @raulcd Would you like to revisit this? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
raulcd commented on PR #45869: URL: https://github.com/apache/arrow/pull/45869#issuecomment-2740848849 CI failures are unrelated. Happy to rebase once they are fixed. @pitrou is something like this what you had in mind? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
pitrou commented on code in PR #45869: URL: https://github.com/apache/arrow/pull/45869#discussion_r2006056322 ## cpp/src/arrow/util/bitmap_ops.h: ## @@ -147,6 +147,31 @@ bool OptionalBitmapEquals(const std::shared_ptr& left, int64_t left_offs const std::shared_ptr& right, int64_t right_offset, int64_t length); +/// \brief Do a "bitmap and" on right and left buffers starting at +/// 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. Review Comment: And this would be even more useful as: ```c++ Result> OptionalBitmapAnd( MemoryPool* pool, const std::shared_ptr& left, int64_t left_offset, const std::shared_ptr& 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
pitrou commented on code in PR #45869: URL: https://github.com/apache/arrow/pull/45869#discussion_r2006056322 ## cpp/src/arrow/util/bitmap_ops.h: ## @@ -147,6 +147,31 @@ bool OptionalBitmapEquals(const std::shared_ptr& left, int64_t left_offs const std::shared_ptr& right, int64_t right_offset, int64_t length); +/// \brief Do a "bitmap and" on right and left buffers starting at +/// 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. Review Comment: And this would be even more useful as: ```c++ Result> OptionalBitmapAnd( MemoryPool* pool, const std::shared_ptr& left, int64_t left_offset, const std::shared_ptr& right, int64_t right_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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
pitrou commented on code in PR #45869: URL: https://github.com/apache/arrow/pull/45869#discussion_r2006052888 ## cpp/src/arrow/util/bitmap_ops.h: ## @@ -147,6 +147,31 @@ bool OptionalBitmapEquals(const std::shared_ptr& left, int64_t left_offs const std::shared_ptr& right, int64_t right_offset, int64_t length); +/// \brief Do a "bitmap and" on right and left buffers starting at +/// 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. Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-45819: [C++] Add OptionalBitmapAnd utility function [arrow]
github-actions[bot] commented on PR #45869: URL: https://github.com/apache/arrow/pull/45869#issuecomment-2740478396 Revision: 41888412fc0db852ea132c6066faaebea6d869bb Submitted crossbow builds: [ursacomputing/crossbow @ actions-fd8ca4245c](https://github.com/ursacomputing/crossbow/branches/all?query=actions-fd8ca4245c) |Task|Status| ||--| |example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/13970600315/job/3907620)| |example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/13970600959/job/3910177)| |example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/13970600395/job/3907348)| |test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13970599769/job/3905498)| |test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/13970600325/job/3907426)| |test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13970600773/job/3910087)| |test-conda-cpp-meson|[](https://github.com/ursacomputing/crossbow/actions/runs/13970600376/job/3907761)| |test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/13970600580/job/3908805)| |test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/13970599745/job/3905526)| |test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/13970601088/job/3910599)| |test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/13970600987/job/3910500)| |test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13970601173/job/3911617)| |test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13970599747/job/3905575)| |test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/13970600732/job/3910047)| |test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/13970600815/job/3910159)| |test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/13970601284/job/3911695)| |test-ubuntu-22.04-cpp-no-threading|[