arrow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Wes McKinney <wesmck...@gmail.com>
Subject Re: [DISCUSS] Result vs Status
Date Wed, 30 Oct 2019 20:25:03 GMT
Returning to this discussion.

Here is my position on the matter since this was brought up on the
sync call today

* For internal / non-public and pseudo-non-public APIs that have
return/out values
  - Use Result or Status at discretion of the developer, but Result<T>
is preferable

* For new public APIs with return/out values
  - Prefer Result<T> unless a Status-based API seems definitely less
awkward in real world use. I have to say that I'm skeptical about the
relative usability of std::tuple outputs and don't think we should
force the use of Result<T> for technical purity reasons

* For existing Status APIs with return values
  - Incrementally add Result<T> APIs and deprecate Status-based APIs.
Maintain deprecated Status APIs for ~2 major releases

On Thu, Oct 24, 2019 at 5:16 PM Omer F. Ozarslan <omer@utdallas.edu> wrote:
>
> Hi Micah,
>
> You're right. Quite possible that clang-query counted same function
> separately for each include in each file. (I was iterating each file
> separately, but providing all of them at once didn't change the result
> either.)
>
> It's cool and wrong, so not very useful apparently. :-)
>
> Best,
> Omer
>
> On Thu, Oct 24, 2019 at 4:51 PM Micah Kornfield <emkornfield@gmail.com> wrote:
> >
> > Hi Omer,
> > I think this is really cool.  It is quite possible it was underestimated (I agree
about line lengths), but I think the clang query is double counting somehow.
> >
> > For instance:
> >
> > "grep -r Status *" only returns ~9000 results in total for me.
> >
> > Similarly using grep for "FinishTyped" returns 18 results for me.  Searching through
the log that you linked seems to return 450 (for "Status FinishTyped").
> >
> > It is quite possible, I'm doing something naive with grep.
> >
> > Thanks,
> > Micah
> >
> > On Thu, Oct 24, 2019 at 2:41 PM Omer F. Ozarslan <omer@utdallas.edu> wrote:
> >>
> >> Forgot to mention most of those lines are longer than line width while
> >> out is usually (always?) last parameter, so probably that's why grep
> >> possibly underestimates their number.
> >>
> >> On Thu, Oct 24, 2019 at 4:33 PM Omer F. Ozarslan <omer@utdallas.edu> wrote:
> >> >
> >> > Hi,
> >> >
> >> > I don't have much experience on customized clang-tidy plugins, but
> >> > this might be a good use case for such a plugin from what I read here
> >> > and there (frankly this was a good excuse for me to have a look at
> >> > clang tooling as well). I wanted to ensure it isn't obviously overkill
> >> > before this suggestion: Running a clang query which lists functions
> >> > returning `arrow::Status` and taking a pointer parameter named `out`
> >> > showed that there are 13947 such functions in `cpp/src/**/*.h`. [1]
> >> >
> >> > I checked logs and it seemed legitimate to me, but please check it in
> >> > case I missed something. If that's the case, it might be tedious to do
> >> > this work manually.
> >> >
> >> > [1]: https://gist.github.com/ozars/ecbb1b8acd4a57ba4721c1965f83f342
> >> > (Note that the log file is shown as truncated by github after ~30k
> >> > lines)
> >> >
> >> > Best,
> >> > Omer
> >> >
> >> >
> >> >
> >> > On Wed, Oct 23, 2019 at 9:23 PM Micah Kornfield <emkornfield@gmail.com>
wrote:
> >> > >
> >> > > OK, it sounds like people want Result<T> (at least in some circumstances).
> >> > > Any thoughts on migrating old APIs and what to do for new APIs going
> >> > > forward?
> >> > >
> >> > > A very rough approximation [1] yields the following counts by module:
> >> > >
> >> > >  853 arrow
> >> > >
> >> > >   17 gandiva
> >> > >
> >> > >   25 parquet
> >> > >
> >> > >   50 plasma
> >> > >
> >> > >
> >> > >
> >> > > [1] grep -r Status cpp/src/* |grep ".h:" | grep "\\*" |grep -v Accept
|sed
> >> > > s/:.*// | cut -f3 -d/ |sort
> >> > >
> >> > >
> >> > > Thanks,
> >> > >
> >> > > Micah
> >> > >
> >> > >
> >> > >
> >> > > On Sat, Oct 19, 2019 at 7:50 PM Francois Saint-Jacques <
> >> > > fsaintjacques@gmail.com> wrote:
> >> > >
> >> > > > As mentioned, Result<T> is an improvement for function
which returns a
> >> > > > single value, e.g. Make/Factory-like. My vote goes Result<T>
for such
> >> > > > case. For multiple return types, we have std::tuple like Antoine
> >> > > > proposed.
> >> > > >
> >> > > > François
> >> > > >
> >> > > > On Fri, Oct 18, 2019 at 9:19 PM Antoine Pitrou <antoine@python.org>
wrote:
> >> > > > >
> >> > > > >
> >> > > > > Le 18/10/2019 à 20:58, Wes McKinney a écrit :
> >> > > > > > I'm definitely uncomfortable with the idea of deprecating
Status.
> >> > > > > >
> >> > > > > > We have a few kinds of functions that can fail:
> >> > > > > >
> >> > > > > > 1. Functions with no "out" arguments
> >> > > > > > 2. Functions with one out argument
> >> > > > > > 3. Functions with multiple out arguments
> >> > > > > >
> >> > > > > > IMHO functions in category 2 are the best candidates
for utilizing
> >> > > > > > Status. In some cases, Case 3 may be more usable Result-based,
but it
> >> > > > > > can also create more work (or confusion) on the part
of the developer,
> >> > > > > > either
> >> > > > > >
> >> > > > > > * The T in Result<T> has to be a struct-like
value that transports
> >> > > > > > multiple pieces of data
> >> > > > >
> >> > > > > The T can be a std::tuple though, so you need not necessarily
define a
> >> > > > > dedicated struct type for a single API's return value.
> >> > > > >
> >> > > > >  > Can't say I'm thrilled about having Result<void>
or similar for Case
> >> > > > >  > 1-type functions (if I'm understanding what would
be the solution
> >> > > > >  > there).
> >> > > > >
> >> > > > > Agreed.
> >> > > > >
> >> > > > > Regards
> >> > > > >
> >> > > > > Antoine.
> >> > > >

Mime
View raw message