You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "taepper (via GitHub)" <gi...@apache.org> on 2026/07/30 06:06:02 UTC
[PR] GH-50703: do not use throwing api [arrow]
taepper opened a new pull request, #50732:
URL: https://github.com/apache/arrow/pull/50732
### Rationale for this change
The CI broke
### What changes are included in this PR?
This changes `json_write_internal.cc` and `from_string.cc` to use the non-throwing simdjson api
### Are these changes tested?
Yes
### Are there any user-facing changes?
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: do not use throwing api [arrow]
Posted by "taepper (via GitHub)" <gi...@apache.org>.
taepper commented on code in PR #50732:
URL: https://github.com/apache/arrow/pull/50732#discussion_r3681415758
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -102,7 +102,14 @@ void JsonWriter::Null() {
needs_comma_ = true;
}
-std::string_view JsonWriter::GetString() const { return builder_.view().value(); }
+Result<std::string_view> JsonWriter::GetString() const {
+ std::string_view view;
+ if (auto error = builder_.view().get(view); error != simdjson::SUCCESS) {
+ return Status::OutOfMemory(
+ "OutOfMemory when allocating buffer to serialize json to string");
Review Comment:
Ah, good point. I amend it to test explicitly for the correct error type and throw another message otherwise
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: [C++] Do not use throwing api [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50732:
URL: https://github.com/apache/arrow/pull/50732#issuecomment-5129278394
The JNI macOS compilation error may be related but this PR still improves on the statu quo, so I'll merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: do not use throwing api [arrow]
Posted by "taepper (via GitHub)" <gi...@apache.org>.
taepper commented on code in PR #50732:
URL: https://github.com/apache/arrow/pull/50732#discussion_r3681155671
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -102,7 +102,13 @@ void JsonWriter::Null() {
needs_comma_ = true;
}
-std::string_view JsonWriter::GetString() const { return builder_.view().value(); }
+std::string_view JsonWriter::GetString() const {
+ std::string_view view;
+ if (builder_.view().get(view) != simdjson::SUCCESS) {
+ return {};
+ }
Review Comment:
The error in `builder_.view()` can only trigger in `OutOfMemory` cases. Should I thread the error through (will also change the api of `fixed_shape_tensor.cc` and `opaque.cc`), or can I use `ARROW_CHECK_OK` in the users for a non-recoverable error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: do not use throwing api [arrow]
Posted by "taepper (via GitHub)" <gi...@apache.org>.
taepper commented on code in PR #50732:
URL: https://github.com/apache/arrow/pull/50732#discussion_r3681334632
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -102,7 +102,14 @@ void JsonWriter::Null() {
needs_comma_ = true;
}
-std::string_view JsonWriter::GetString() const { return builder_.view().value(); }
+Result<std::string_view> JsonWriter::GetString() const {
+ std::string_view view;
+ if (auto error = builder_.view().get(view); error != simdjson::SUCCESS) {
+ return Status::OutOfMemory(
+ "OutOfMemory when allocating buffer to serialize json to string");
Review Comment:
It does not. As the error is just an enum, it will only add the message of:
```{ OUT_OF_CAPACITY, /*message=*/"OUT_OF_CAPACITY: The capacity was exceeded, we cannot allocate enough memory."}```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: do not use throwing api [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50732:
URL: https://github.com/apache/arrow/pull/50732#discussion_r3681394192
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -102,7 +102,14 @@ void JsonWriter::Null() {
needs_comma_ = true;
}
-std::string_view JsonWriter::GetString() const { return builder_.view().value(); }
+Result<std::string_view> JsonWriter::GetString() const {
+ std::string_view view;
+ if (auto error = builder_.view().get(view); error != simdjson::SUCCESS) {
+ return Status::OutOfMemory(
+ "OutOfMemory when allocating buffer to serialize json to string");
Review Comment:
I was thinking about the unlikely case (a future simdjson version perhaps?) where we get something other than `OUT_OF_CAPACITY`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: do not use throwing api [arrow]
Posted by "taepper (via GitHub)" <gi...@apache.org>.
taepper commented on code in PR #50732:
URL: https://github.com/apache/arrow/pull/50732#discussion_r3680592277
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -102,7 +102,13 @@ void JsonWriter::Null() {
needs_comma_ = true;
}
-std::string_view JsonWriter::GetString() const { return builder_.view().value(); }
+std::string_view JsonWriter::GetString() const {
+ std::string_view view;
+ if (builder_.view().get(view) != simdjson::SUCCESS) {
+ return {};
+ }
Review Comment:
I quickly investigated. This change would touch at least:
```
cpp/src/arrow/extension/fixed_shape_tensor.cc
cpp/src/arrow/extension/opaque.cc
cpp/src/arrow/extension/variable_shape_tensor.cc
cpp/src/arrow/integration/json_integration.cc
cpp/src/arrow/integration/json_integration_test.cc
cpp/src/arrow/json/json_writer_internal.cc
cpp/src/arrow/json/json_writer_internal.h
cpp/src/arrow/json/json_writer_internal_test.cc
```
and might conflict with other work by @Reranko05 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: [C++] Do not use throwing api [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #50732:
URL: https://github.com/apache/arrow/pull/50732
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: do not use throwing api [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50732:
URL: https://github.com/apache/arrow/pull/50732#discussion_r3681289454
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -102,7 +102,14 @@ void JsonWriter::Null() {
needs_comma_ = true;
}
-std::string_view JsonWriter::GetString() const { return builder_.view().value(); }
+Result<std::string_view> JsonWriter::GetString() const {
+ std::string_view view;
+ if (auto error = builder_.view().get(view); error != simdjson::SUCCESS) {
+ return Status::OutOfMemory(
+ "OutOfMemory when allocating buffer to serialize json to string");
Review Comment:
Can we still append the simdjson error message (in case it would say something more specific)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: do not use throwing api [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50732:
URL: https://github.com/apache/arrow/pull/50732#discussion_r3680637359
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -102,7 +102,13 @@ void JsonWriter::Null() {
needs_comma_ = true;
}
-std::string_view JsonWriter::GetString() const { return builder_.view().value(); }
+std::string_view JsonWriter::GetString() const {
+ std::string_view view;
+ if (builder_.view().get(view) != simdjson::SUCCESS) {
+ return {};
+ }
Review Comment:
Well, I don't think we want to silence errors here, so I think we should do the change indeed :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: do not use throwing api [arrow]
Posted by "taepper (via GitHub)" <gi...@apache.org>.
taepper commented on code in PR #50732:
URL: https://github.com/apache/arrow/pull/50732#discussion_r3681213991
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -102,7 +102,13 @@ void JsonWriter::Null() {
needs_comma_ = true;
}
-std::string_view JsonWriter::GetString() const { return builder_.view().value(); }
+std::string_view JsonWriter::GetString() const {
+ std::string_view view;
+ if (builder_.view().get(view) != simdjson::SUCCESS) {
+ return {};
+ }
Review Comment:
Changing `ExtensionType::Serialize()` would be a bigger api breaking change. In those cases I added `ARROW_CHECK_OK` statements (even though it is not in test-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: do not use throwing api [arrow]
Posted by "taepper (via GitHub)" <gi...@apache.org>.
taepper commented on code in PR #50732:
URL: https://github.com/apache/arrow/pull/50732#discussion_r3680592277
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -102,7 +102,13 @@ void JsonWriter::Null() {
needs_comma_ = true;
}
-std::string_view JsonWriter::GetString() const { return builder_.view().value(); }
+std::string_view JsonWriter::GetString() const {
+ std::string_view view;
+ if (builder_.view().get(view) != simdjson::SUCCESS) {
+ return {};
+ }
Review Comment:
I quickly investigated. This change would touch at least:
```
(cpp/src/arrow/json/json_writer_internal.cc)
cpp/src/arrow/extension/fixed_shape_tensor.cc
cpp/src/arrow/extension/opaque.cc
cpp/src/arrow/extension/variable_shape_tensor.cc
cpp/src/arrow/integration/json_integration.cc
cpp/src/arrow/integration/json_integration_test.cc
cpp/src/arrow/json/json_writer_internal.h
cpp/src/arrow/json/json_writer_internal_test.cc
```
and might conflict with other work by @Reranko05 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50730: [C++] Do not use throwing api [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50732:
URL: https://github.com/apache/arrow/pull/50732#issuecomment-5153309199
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 86f8d42b68f6b4624cf544aec23fe9cba1346431.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/91415169189) has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org