GH-46439: [C++] Use result pattern for all FromJSONString Helpers#46696
Conversation
|
Thanks @kou for the quick review. I fixed what I could and left one conversation open in #46696 (comment). I'll hold off on rebasing to fix the merge conflict until after review is over. |
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
c22be6c to
ed41702
Compare
|
Rebased, all outstanding review comments addressed. |
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Not sure how CI didn't catch this.
|
Looks like there are some issues with gdb.cc I need to figure out. I get this, |
|
I think the gdb.cc errors are fixed in 641839f. I realize we can't use those helpers and that it's fine to just ValueOrDie (directly or via |
(can't test on macOS)
|
CI looks good now, I had two fix a couple of issues with gdb.cc. @kou do you want to take one more look at that file before we merge? |
|
Thank you as always @kou |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit c85c3d0. There were 71 benchmark results with an error:
There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 53 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…rs (apache#46696) ### Rationale for this change apache#45908 brought these helpers into the public API but didn't consider changes to their API. This PR makes all the helpers use the standard Result-pattern to make them more ergonomic. We can do this now without a breaking change because this and apache#45908 will be part of Arrow 21. ### What changes are included in this PR? - Refactored all FromJSONString helpers to use the Result pattern (instead of using outparams) ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#46439 Lead-authored-by: Bryce Mecum <petridish@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
#45908 brought these helpers into the public API but didn't consider changes to their API. This PR makes all the helpers use the standard Result-pattern to make them more ergonomic. We can do this now without a breaking change because this and #45908 will be part of Arrow 21.
What changes are included in this PR?
Are these changes tested?
Yes.
Are there any user-facing changes?
No.