You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@arrow.apache.org by Benjamin Kietzman <be...@gmail.com> on 2021/03/11 21:24:27 UTC
[DISCUSS][C++] Reduce usage of KernelContext in compute::
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
Re: [DISCUSS][C++] Reduce usage of KernelContext in compute::
Posted by Benjamin Kietzman <be...@gmail.com>.
Incorrect link last time, sorry:
https://issues.apache.org/jira/browse/ARROW-11990
On Tue, Mar 16, 2021 at 4:19 PM Benjamin Kietzman <be...@gmail.com>
wrote:
> I've opened https://issues.apache.org/jira/browse/ARROW-11945
> to track this improvement.
>
> On Tue, Mar 16, 2021 at 3:40 PM Wes McKinney <we...@gmail.com> wrote:
>
>> It seems fine to me to return Result from KernelInit (as long as a
>> context of some kind of still passed in to the function to have the
>> possibility of passing other configuration bits), and it would make
>> development more convenient to use a common error handling strategy in
>> more places
>>
>> On Fri, Mar 12, 2021 at 8:52 AM Antoine Pitrou <an...@python.org>
>> wrote:
>> >
>> >
>> > I wouldn't mind changing those APIs to return a Status.
>> > I'll also note that KernelContext::SetStatus() isn't thread-safe.
>> >
>> > Regards
>> >
>> > Antoine.
>> >
>> >
>> > Le 12/03/2021 à 11:40, Benjamin Kietzman a écrit :
>> > > 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 <yi...@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
>> > >>>
>> > >>
>> > >
>>
>
Re: [DISCUSS][C++] Reduce usage of KernelContext in compute::
Posted by Benjamin Kietzman <be...@gmail.com>.
I've opened https://issues.apache.org/jira/browse/ARROW-11945
to track this improvement.
On Tue, Mar 16, 2021 at 3:40 PM Wes McKinney <we...@gmail.com> wrote:
> It seems fine to me to return Result from KernelInit (as long as a
> context of some kind of still passed in to the function to have the
> possibility of passing other configuration bits), and it would make
> development more convenient to use a common error handling strategy in
> more places
>
> On Fri, Mar 12, 2021 at 8:52 AM Antoine Pitrou <an...@python.org> wrote:
> >
> >
> > I wouldn't mind changing those APIs to return a Status.
> > I'll also note that KernelContext::SetStatus() isn't thread-safe.
> >
> > Regards
> >
> > Antoine.
> >
> >
> > Le 12/03/2021 à 11:40, Benjamin Kietzman a écrit :
> > > 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 <yi...@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
> > >>>
> > >>
> > >
>
Re: [DISCUSS][C++] Reduce usage of KernelContext in compute::
Posted by Wes McKinney <we...@gmail.com>.
It seems fine to me to return Result from KernelInit (as long as a
context of some kind of still passed in to the function to have the
possibility of passing other configuration bits), and it would make
development more convenient to use a common error handling strategy in
more places
On Fri, Mar 12, 2021 at 8:52 AM Antoine Pitrou <an...@python.org> wrote:
>
>
> I wouldn't mind changing those APIs to return a Status.
> I'll also note that KernelContext::SetStatus() isn't thread-safe.
>
> Regards
>
> Antoine.
>
>
> Le 12/03/2021 à 11:40, Benjamin Kietzman a écrit :
> > 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 <yi...@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
> >>>
> >>
> >
Re: [DISCUSS][C++] Reduce usage of KernelContext in compute::
Posted by Antoine Pitrou <an...@python.org>.
I wouldn't mind changing those APIs to return a Status.
I'll also note that KernelContext::SetStatus() isn't thread-safe.
Regards
Antoine.
Le 12/03/2021 à 11:40, Benjamin Kietzman a écrit :
> 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 <yi...@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
>>>
>>
>
Re: [DISCUSS][C++] Reduce usage of KernelContext in compute::
Posted by Benjamin Kietzman <be...@gmail.com>.
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 <yi...@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
> >
>
Re: [DISCUSS][C++] Reduce usage of KernelContext in compute::
Posted by Yibo Cai <yi...@arm.com>.
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
>