bkietz commented on a change in pull request #8728:
URL: https://github.com/apache/arrow/pull/8728#discussion_r528781337
##########
File path: cpp/src/arrow/compute/kernels/scalar_boolean.cc
##########
@@ -61,40 +59,78 @@ void ComputeKleene(ComputeWord&& compute_word,
KernelContext* ctx, const ArrayDa
++i;
};
- if (right.null_count == 0 || left.null_count == 0) {
- if (left.null_count == 0) {
- // ensure only bitmaps[RIGHT_VALID].buffer might be null
- std::swap(bitmaps[LEFT_VALID], bitmaps[RIGHT_VALID]);
- std::swap(bitmaps[LEFT_DATA], bitmaps[RIGHT_DATA]);
- }
- // override bitmaps[RIGHT_VALID] to make it safe for Visit()
+ if (right.null_count == 0) {
+ // bitmaps[RIGHT_VALID] might be null; override to make it safe for Visit()
bitmaps[RIGHT_VALID] = bitmaps[RIGHT_DATA];
-
Bitmap::VisitWords(bitmaps, [&](std::array<uint64_t, 4> words) {
apply(words[LEFT_VALID], words[LEFT_DATA], ~uint64_t(0),
words[RIGHT_DATA]);
});
- } else {
+ return;
+ }
+
+ if (left.null_count == 0) {
+ // bitmaps[LEFT_VALID] might be null; override to make it safe for Visit()
+ bitmaps[LEFT_VALID] = bitmaps[LEFT_DATA];
Bitmap::VisitWords(bitmaps, [&](std::array<uint64_t, 4> words) {
- apply(words[LEFT_VALID], words[LEFT_DATA], words[RIGHT_VALID],
words[RIGHT_DATA]);
+ apply(~uint64_t(0), words[LEFT_DATA], words[RIGHT_VALID],
words[RIGHT_DATA]);
});
+ return;
}
+
+ DCHECK(left.null_count != 0 || right.null_count != 0);
+ Bitmap::VisitWords(bitmaps, [&](std::array<uint64_t, 4> words) {
+ apply(words[LEFT_VALID], words[LEFT_DATA], words[RIGHT_VALID],
words[RIGHT_DATA]);
+ });
+}
+
+inline BooleanScalar InvertScalar(const Scalar& in) {
+ return in.is_valid ? BooleanScalar(!checked_cast<const
BooleanScalar&>(in).value)
+ : BooleanScalar();
+}
+
+inline Bitmap GetBitmap(const ArrayData& arr, int index) {
+ return Bitmap{arr.buffers[index], arr.offset, arr.length};
}
struct Invert {
static void Call(KernelContext* ctx, const Scalar& in, Scalar* out) {
- if (in.is_valid) {
- checked_cast<BooleanScalar*>(out)->value =
- !checked_cast<const BooleanScalar&>(in).value;
- }
+ *checked_cast<BooleanScalar*>(out) = InvertScalar(in);
}
static void Call(KernelContext* ctx, const ArrayData& in, ArrayData* out) {
- ::arrow::internal::InvertBitmap(in.buffers[1]->data(), in.offset,
in.length,
- out->buffers[1]->mutable_data(),
out->offset);
+ GetBitmap(*out, 1).CopyFromInverted(GetBitmap(in, 1));
}
};
-struct And {
+template <typename Op>
+struct Commutative {
+ static void Call(KernelContext* ctx, const Scalar& left, const ArrayData&
right,
+ ArrayData* out) {
+ Op::Call(ctx, right, left, out);
+ }
+};
+
+struct And : Commutative<And> {
+ using Commutative<And>::Call;
+
+ static void Call(KernelContext* ctx, const Scalar& left, const Scalar& right,
+ Scalar* out) {
+ if (left.is_valid && right.is_valid) {
+ checked_cast<BooleanScalar*>(out)->value =
+ checked_cast<const BooleanScalar&>(left).value &&
+ checked_cast<const BooleanScalar&>(right).value;
+ }
+ }
+
+ static void Call(KernelContext* ctx, const ArrayData& left, const Scalar&
right,
+ ArrayData* out) {
+ if (!right.is_valid) return; // all null case
+
+ return checked_cast<const BooleanScalar&>(right).value
+ ? GetBitmap(*out, 1).CopyFrom(GetBitmap(left, 1))
Review comment:
Reusing the bitmap like this will make it harder to refactor Kleene
kernels to support writing into slices, which (I'm guessing) would be the
greater performance win. In any case, it can wait till follow up
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
