aocsa commented on a change in pull request #11759:
URL: https://github.com/apache/arrow/pull/11759#discussion_r755060093
##########
File path: cpp/src/arrow/compute/kernels/vector_replace.cc
##########
@@ -73,6 +73,10 @@ struct CopyArrayBitmap {
const uint8_t* in_bitmap;
int64_t in_offset;
+ const uint8_t* GetInBitmap() const {
+ return in_bitmap;
+ }
+
Review comment:
You can remove this method, this is a struct so properties are public
by default. I recommend to check this reference:
https://www.educative.io/collection/page/10370001/5111386763952128/6533026123087872
##########
File path: cpp/src/arrow/compute/kernels/vector_replace.cc
##########
@@ -83,11 +87,19 @@ struct CopyArrayBitmap {
BitUtil::SetBitTo(out_bitmap, out_offset,
BitUtil::GetBit(in_bitmap, in_offset + offset));
}
+
+ void SetBitTrue(uint8_t* out_bitmap, int64_t out_offset, int64_t offset)
const {
+ BitUtil::SetBitTo(out_bitmap, out_offset, true);
+ }
};
struct CopyScalarBitmap {
const bool is_valid;
+ const uint8_t* GetInBitmap() const {
+ return nullptr;
+ }
+
Review comment:
So maybe instead of creating this weird method, you can create a
property like this:
```cpp
const static uint8_t* in_bitmap = nullptr;
```
##########
File path: cpp/src/arrow/compute/kernels/vector_replace.cc
##########
@@ -83,11 +87,19 @@ struct CopyArrayBitmap {
BitUtil::SetBitTo(out_bitmap, out_offset,
BitUtil::GetBit(in_bitmap, in_offset + offset));
}
+
+ void SetBitTrue(uint8_t* out_bitmap, int64_t out_offset, int64_t offset)
const {
+ BitUtil::SetBitTo(out_bitmap, out_offset, true);
+ }
Review comment:
I would suggest to change method name to `SetBitmapToTrue`
##########
File path: cpp/src/arrow/compute/kernels/vector_replace_test.cc
##########
@@ -223,6 +223,22 @@ TYPED_TEST(TestReplaceNumeric, ReplaceWithMask) {
this->mask("[false, false, null, null, true, true]"),
this->array("[10, null]"),
this->array("[null, null, null, null, 10, null]"));
+ this->Assert(ReplaceWithMask, this->array("[1, null, 1]"),
+ this->mask("[false, true, false]"),
+ this->array("[7]"),
+ this->array("[1, 7, 1]"));
+ this->Assert(ReplaceWithMask, this->array("[1, null, 1, 7]"),
+ this->mask("[false, true, false, true]"),
+ this->array("[7, 20]"),
+ this->array("[1, 7, 1, 20]"));
+ this->Assert(ReplaceWithMask, this->array("[1, 2, 3, 4]"),
+ this->mask("[false, true, false, true]"),
+ this->array("[null, null]"),
+ this->array("[1, null, 3, null]"));
+ this->Assert(ReplaceWithMask, this->array("[null, 2, 3, 4]"),
+ this->mask("[true, true, false, true]"),
+ this->array("[1, null, null]"),
Review comment:
I would suggest to move all these new tests to a new test function to
clarify the purpose of this PR.
TYPED_TEST(TestReplaceNumeric, `ReplaceWithMaskForNullValuesAndMaskEnabled`)
##########
File path: cpp/src/arrow/compute/kernels/vector_replace.cc
##########
@@ -96,6 +108,10 @@ struct CopyScalarBitmap {
void SetBit(uint8_t* out_bitmap, int64_t out_offset, int64_t offset) const {
BitUtil::SetBitTo(out_bitmap, out_offset, is_valid);
}
+
+ void SetBitTrue(uint8_t* out_bitmap, int64_t out_offset, int64_t offset)
const {
+ BitUtil::SetBitTo(out_bitmap, out_offset, true);
+ }
Review comment:
Use same method name here as suggested before
--
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]
