lidavidm commented on a change in pull request #11759:
URL: https://github.com/apache/arrow/pull/11759#discussion_r757133536
##########
File path: cpp/src/arrow/compute/kernels/vector_replace.cc
##########
@@ -189,20 +201,26 @@ Status ReplaceWithArrayMask(KernelContext* ctx, const
ArrayData& array,
const ArrayData& array_repl = *replacements.array();
ReplaceWithArrayMaskImpl<Functor>(
array, mask, array_repl, replacements_bitmap,
- CopyArrayBitmap{replacements_bitmap ? array_repl.buffers[0]->data() :
nullptr,
- array_repl.offset},
+ CopyArrayBitmap{
+ (!!array_repl.buffers[0]) ? array_repl.buffers[0]->data() :
nullptr,
+ array_repl.offset},
mask_bitmap, mask_values, out_bitmap, out_values, out_offset);
} else {
const Scalar& scalar_repl = *replacements.scalar();
ReplaceWithArrayMaskImpl<Functor>(array, mask, scalar_repl,
replacements_bitmap,
- CopyScalarBitmap{scalar_repl.is_valid},
mask_bitmap,
- mask_values, out_bitmap, out_values,
out_offset);
+ CopyScalarBitmap{scalar_repl.is_valid,
nullptr},
+ mask_bitmap, mask_values, out_bitmap,
out_values,
+ out_offset);
}
if (mask.MayHaveNulls()) {
arrow::internal::BitmapAnd(out_bitmap, out_offset, mask.buffers[0]->data(),
mask.offset, array.length, out_offset,
out_bitmap);
}
+ if (!output->GetNullCount()) {
+ output->null_count = 0;
+ output->buffers[0] = NULLPTR;
Review comment:
I think with preallocated bitmaps like this, we should not delete the
output buffer like this (it breaks `can_write_into_slices`).
##########
File path: cpp/src/arrow/compute/kernels/vector_replace.cc
##########
@@ -96,6 +105,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 SetBitToTrue(uint8_t* out_bitmap, int64_t out_offset, int64_t offset)
const {
Review comment:
Nor is this.
##########
File path: cpp/src/arrow/compute/kernels/vector_replace.cc
##########
@@ -81,12 +83,19 @@ struct CopyArrayBitmap {
void SetBit(uint8_t* out_bitmap, int64_t out_offset, int64_t offset) const {
BitUtil::SetBitTo(out_bitmap, out_offset,
- BitUtil::GetBit(in_bitmap, in_offset + offset));
+ in_bitmap ? BitUtil::GetBit(in_bitmap, in_offset +
offset) : true);
+ }
+
+ void SetBitToTrue(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* in_bitmap;
+
+ const uint8_t* GetInBitmap() const { return in_bitmap; }
Review comment:
This isn't used anymore.
##########
File path: cpp/src/arrow/compute/kernels/vector_replace.cc
##########
@@ -189,20 +201,26 @@ Status ReplaceWithArrayMask(KernelContext* ctx, const
ArrayData& array,
const ArrayData& array_repl = *replacements.array();
ReplaceWithArrayMaskImpl<Functor>(
array, mask, array_repl, replacements_bitmap,
- CopyArrayBitmap{replacements_bitmap ? array_repl.buffers[0]->data() :
nullptr,
- array_repl.offset},
+ CopyArrayBitmap{
+ (!!array_repl.buffers[0]) ? array_repl.buffers[0]->data() :
nullptr,
+ array_repl.offset},
mask_bitmap, mask_values, out_bitmap, out_values, out_offset);
} else {
const Scalar& scalar_repl = *replacements.scalar();
ReplaceWithArrayMaskImpl<Functor>(array, mask, scalar_repl,
replacements_bitmap,
- CopyScalarBitmap{scalar_repl.is_valid},
mask_bitmap,
- mask_values, out_bitmap, out_values,
out_offset);
+ CopyScalarBitmap{scalar_repl.is_valid,
nullptr},
+ mask_bitmap, mask_values, out_bitmap,
out_values,
+ out_offset);
}
if (mask.MayHaveNulls()) {
arrow::internal::BitmapAnd(out_bitmap, out_offset, mask.buffers[0]->data(),
mask.offset, array.length, out_offset,
out_bitmap);
}
+ if (!output->GetNullCount()) {
+ output->null_count = 0;
+ output->buffers[0] = NULLPTR;
Review comment:
(You can see this on line 503 - the kernel specifies the output validity
buffer can be preallocated.)
##########
File path: cpp/src/arrow/compute/kernels/vector_replace.cc
##########
@@ -119,10 +132,10 @@ void ReplaceWithArrayMaskImpl(const ArrayData& array,
const ArrayData& mask,
// Copy from replacement array
Functor::CopyData(*array.type, out_values, out_offset + write_offset,
replacements,
replacements_offset, block.length);
- if (replacements_bitmap) {
+ if (replacements_bitmap && Functor::HasBitmap(replacements)) {
Review comment:
`replacements_bitmap` is already a test for whether we have a validity
bitmap in the replacements array - why another check?
--
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]
