bkietz commented on a change in pull request #8728:
URL: https://github.com/apache/arrow/pull/8728#discussion_r528768308
##########
File path: cpp/src/arrow/compute/exec.cc
##########
@@ -272,79 +287,61 @@ class NullPropagator {
return Status::OK();
}
- Result<bool> ShortCircuitIfAllNull() {
- // An all-null value (scalar null or all-null array) gives us a short
- // circuit opportunity
- bool is_all_null = false;
- std::shared_ptr<Buffer> all_null_bitmap;
+ Status AllNullShortCircuit() {
+ // OK, the output should be all null
+ output_->null_count = output_->length;
+
+ if (bitmap_preallocated_) {
+ BitUtil::SetBitsTo(bitmap_, output_->offset, output_->length, false);
+ return Status::OK();
+ }
// Walk all the values with nulls instead of breaking on the first in case
// we find a bitmap that can be reused in the non-preallocated case
- for (const Datum* value : values_with_nulls_) {
- if (value->type()->id() == Type::NA) {
- // No bitmap
- is_all_null = true;
- } else if (value->kind() == Datum::ARRAY) {
- const ArrayData& arr = *value->array();
- if (arr.null_count.load() == arr.length) {
- // Pluck the all null bitmap so we can set it in the output if it was
- // not pre-allocated
- all_null_bitmap = arr.buffers[0];
- is_all_null = true;
- }
- } else {
- // Scalar
- is_all_null = !value->scalar()->is_valid;
+ for (const ArrayData* arr : arrays_with_nulls_) {
+ if (arr->null_count.load() == arr->length && arr->buffers[0] != nullptr)
{
+ // Reuse this all null bitmap
+ output_->buffers[0] = arr->buffers[0];
+ return Status::OK();
}
}
- if (!is_all_null) {
- return false;
- }
- // OK, the output should be all null
- output_->null_count = output_->length;
-
- if (!bitmap_preallocated_ && all_null_bitmap) {
- // If we did not pre-allocate memory, and we observed an all-null bitmap,
- // then we can zero-copy it into the output
- output_->buffers[0] = std::move(all_null_bitmap);
- } else {
- RETURN_NOT_OK(EnsureAllocated());
- BitUtil::SetBitsTo(bitmap_, output_->offset, output_->length, false);
- }
- return true;
+ RETURN_NOT_OK(EnsureAllocated());
+ BitUtil::SetBitsTo(bitmap_, output_->offset, output_->length, false);
+ return Status::OK();
}
Status PropagateSingle() {
// One array
- const ArrayData& arr = *values_with_nulls_[0]->array();
+ const ArrayData& arr = *arrays_with_nulls_[0];
const std::shared_ptr<Buffer>& arr_bitmap = arr.buffers[0];
// Reuse the null count if it's known
output_->null_count = arr.null_count.load();
if (bitmap_preallocated_) {
CopyBitmap(arr_bitmap->data(), arr.offset, arr.length, bitmap_,
output_->offset);
+ return Status::OK();
+ }
+
+ // Two cases when memory was not pre-allocated:
+ //
+ // * Offset is zero: we reuse the bitmap as is
+ // * Offset is nonzero but a multiple of 8: we can slice the bitmap
+ // * Offset is not a multiple of 8: we must allocate and use CopyBitmap
+ //
+ // Keep in mind that output_->offset is not permitted to be nonzero when
+ // the bitmap is not preallocated, and that precondition is asserted
+ // higher in the call stack.
+ if (arr.offset == 0) {
+ output_->buffers[0] = arr_bitmap;
+ } else if (arr.offset % 8 == 0) {
+ output_->buffers[0] =
+ SliceBuffer(arr_bitmap, arr.offset / 8,
BitUtil::BytesForBits(arr.length));
Review comment:
Sounds worthwhile but I'd prefer to do it in 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]
