felipecrv commented on code in PR #41975:
URL: https://github.com/apache/arrow/pull/41975#discussion_r1638523247
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -1034,9 +1034,23 @@ class VectorExecutor : public
KernelExecutorImpl<VectorKernel> {
output_num_buffers_ =
static_cast<int>(output_type_.type->layout().buffers.size());
// Decide if we need to preallocate memory for this kernel
- validity_preallocated_ =
- (kernel_->null_handling != NullHandling::COMPUTED_NO_PREALLOCATE &&
- kernel_->null_handling != NullHandling::OUTPUT_NOT_NULL);
+ validity_preallocated_ = false;
+ if (output_type_.type->id() != Type::NA) {
+ if (kernel_->null_handling == NullHandling::COMPUTED_PREALLOCATE) {
+ // Override the flag if kernel asks for pre-allocation
+ validity_preallocated_ = true;
+ } else if (kernel_->null_handling == NullHandling::INTERSECTION) {
+ bool elide_validity_bitmap = true;
+ for (const auto& arg : batch.values) {
+ auto null_gen = NullGeneralization::Get(arg) ==
NullGeneralization::ALL_VALID;
+
+ // If not all valid, this becomes false
+ elide_validity_bitmap = elide_validity_bitmap && null_gen;
+ }
+ validity_preallocated_ = !elide_validity_bitmap;
+ }
+ }
Review Comment:
I don't think this logic is safe. If `null_handling` is `INTERSECTION` the
function might want to follow a single code path that uses the pre-allocated
buffer to put the result of the bitmaps intersection.
And even if that is not the case (assuming all kernels are perfect at the
moment), this extra logic introduces branching in the possible states of
pre-allocated buffers based on runtime input data and not something statically
defined in the `kernel_` when it's configured.
If you want the speed benefits of not pre-allocating a validity bitmap when
not necessary, declare the kernel as `COMPUTED_NO_PREALLOCATE` and work on the
kernel logic to allocate only when needed. The generic binding and execution
logic should be simple and not depend on values so much. It's too complicated
as it is.
--
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]
