Skip to content

Conversation

@amoeba
Copy link
Member

@amoeba amoeba commented Jun 3, 2025

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?

  • 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.

@kou kou changed the title GH-46439: [C++]: Use result pattern for all FromJSONString Helpers GH-46439: [C++] Use result pattern for all FromJSONString Helpers Jun 3, 2025
@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Jun 3, 2025
@amoeba
Copy link
Member Author

amoeba commented Jun 3, 2025

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.

@amoeba amoeba force-pushed the fix/GH-46439--post-merge-expose-fromjson--use-result-pattern branch from c22be6c to ed41702 Compare June 4, 2025 00:34
@amoeba
Copy link
Member Author

amoeba commented Jun 4, 2025

Rebased, all outstanding review comments addressed.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@github-actions github-actions bot removed the awaiting change review Awaiting change review label Jun 4, 2025
@github-actions github-actions bot added the awaiting merge Awaiting merge label Jun 4, 2025
amoeba and others added 2 commits June 3, 2025 17:50
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Not sure how CI didn't catch this.
@amoeba
Copy link
Member Author

amoeba commented Jun 4, 2025

Looks like there are some issues with gdb.cc I need to figure out. I get this,

[ 24%] Building CXX object CMakeFiles/arrow_python.dir/pyarrow/src/arrow/python/gdb.cc.o
/Users/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/gdb.cc:367:12: error: expected '(' for function-style cast or type construction
  367 |       auto heap_map_scalar,
      |       ~~~~ ^
/Users/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/gdb.cc:369:46: error: use of undeclared identifier 'heap_map_scalar'
  369 |   auto heap_map_scalar_null = MakeNullScalar(heap_map_scalar->type);
      |                                              ^
/Users/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/gdb.cc:482:29: error: expected '(' for function-style cast or type construction
  482 |   ASSERT_OK_AND_ASSIGN(auto col1,
      |                        ~~~~ ^
/Users/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/gdb.cc:485:12: error: expected '(' for function-style cast or type construction
  485 |       auto col2, ChunkedArrayFromJSONString(
      |       ~~~~ ^
/Users/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/gdb.cc:487:43: error: use of undeclared identifier 'col1'
  487 |   auto table = Table::Make(batch_schema, {col1, col2});
      |                                           ^
/Users/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/gdb.cc:487:49: error: use of undeclared identifier 'col2'
  487 |   auto table = Table::Make(batch_schema, {col1, col2});
      |                                                 ^
6 errors generated.

@amoeba
Copy link
Member Author

amoeba commented Jun 4, 2025

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 operator*). Watching CI now.

(can't test on macOS)
@amoeba
Copy link
Member Author

amoeba commented Jun 4, 2025

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?

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@kou kou merged commit c85c3d0 into apache:main Jun 5, 2025
36 of 38 checks passed
@kou kou removed the awaiting merge Awaiting merge label Jun 5, 2025
@amoeba
Copy link
Member Author

amoeba commented Jun 5, 2025

Thank you as always @kou

@conbench-apache-arrow
Copy link

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.

alinaliBQ pushed a commit to Bit-Quill/arrow that referenced this pull request Jun 17, 2025
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants