This seems reasonable to me. Give the impact of the API changes I think it might be worth keeping around for ~3 releases, but I think we are generally slow to delete deprecated APIs anyways.
Any other thoughts on this? i can try to open up some tracking JIRAs for the work involved. On Wed, Oct 30, 2019 at 1:25 PM Wes McKinney <wesmck...@gmail.com> wrote: > 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 <o...@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 <emkornfi...@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 <o...@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 <o...@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 < > emkornfi...@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 < > > >> > > fsaintjacq...@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 < > anto...@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. > > >> > > > >