edponce commented on pull request #12089: URL: https://github.com/apache/arrow/pull/12089#issuecomment-1006577201
@joosthooz I have following initial comments: 1. [ScalarExecutor](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/exec.cc#L635) has two possible preallocation policies ([contiguous](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/exec.cc#L813) and [per-batch](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/exec.cc#L753-L763)) but they both are assigned to `out->value` which is an `ArrayData` (`out` is a `Datum` wrapping either an `Array` or a `Scalar`. I think we need to validate `out` only for the `Array` case. Use [`out.array()`](https://github.com/apache/arrow/blob/master/cpp/src/arrow/datum.h#L190) to access `ArrayData` and its [buffers](https://github.com/apache/arrow/blob/master/cpp/src/arrow/array/data.h#L238-L242). * _Note:_ `buffer[0] is the null bitmap, which does not need this validation bc it has its own nullity policies (see `NullPropagator` and `NullHandling`). 2. The [`VectorExecutor` can also preallocate](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/exec.cc#L873) so it also requires this validation. I think here we would also validate `out`. 3. Kernels establish preallocation policy via `kernel.mem_allocation = MemAllocation::PREALLOCATE` during registration. For example, [here](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_string.cc#L4712-L4717). -- 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]
