My primary point is that using KernelContext to hold error statuses is confusing since there are more places to check for an error condition. In the rest of the c++ library we use RETURN_NOT_OK or ARROW_ASSIGN_OR_RAISE to handle stack unwinding from an error, but in the presence of KernelContext it's also necessary to check KernelContext::HasError.
The specific case of a Kernel::init which must allocate actually provides a good example of this: KernelContext includes helper methods AllocateBuffer and AllocateBitmap which use the context's memory pool. These return Result<>, whose status must then be checked and errors propagated using KernelContext::SetStatus (and not ARROW_ASSIGN_OR_RAISE as in the rest of the c++ library) since Kernel::init doesn't support status returns. IMHO it'd increase readability of kernel code to handle errors uniformly wherever possible. On Fri, Mar 12, 2021, 01:00 Yibo Cai <yibo....@arm.com> wrote: > Beside reporting errors, maybe a kernel wants to allocate memory through > KernelContext::memory_pool [1] in Kernel::init? > I'm not quite sure if this is a valid case. Would like to hear other > comments. > > [1] > https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernel.h#L95 > > Yibo > > On 3/12/21 5:24 AM, Benjamin Kietzman wrote: > > KernelContext is a tuple consisting of a pointers to an ExecContext and > > KernelState > > and an error Status. The context's error Status may be set by compute > > kernels (for > > example when divide-by-zero would occur) rather than returning a Result > as > > in the > > rest of the codebase. IIUC the intent is to avoid branching on always-ok > > Statuses > > for kernels which don't have an error condition (for example addition > > without overflow > > checks). > > > > If there's a motivating performance reason for non standard error > > propagation then > > we should continue using KernelContext wherever we can benefit from it. > > However, > > several other APIs (such as Kernel::init) also use a KernelContext to > > report errors. > > IMHO, it would be better to avoid the added cognitive overhead of > handling > > errors > > through KernelContext outside hot loops which benefit from it. > > > > Am I missing anything? Is there any reason (for example) Kernel::init > > shouldn't just > > return a Result<unique_ptr<KernelState>>? > > > > Ben Kietzman > > >
