Uh oh!
There was an error while loading. Please reload this page.
GH-50654: [C++] Support simdjson without exceptions - #50672
Conversation
Uh oh!
There was an error while loading. Please reload this page.
f94fe05 to
fde573aCompareUh oh!
There was an error while loading. Please reload this page.
fde573a to
92ae650Compare| std::string_view str; | ||
| if (auto error = std::move(str_result).get(str)) { | ||
| return Status::Invalid("Error getting string for key '", key, | ||
| "': ", simdjson::error_message(error)); | ||
| } | ||
| return std::string(str); |
There was a problem hiding this comment.
#50653 adds
template <typename SimdjsonValueType>
Result<SimdjsonValueType> GetJsonAs(sj::value& value) {
that already returns arrow::Results instead of error codes. This might simplify these type-casts
There was a problem hiding this comment.
I wanted to write after this PR is merged. Referenced the wrong PR. Fixed it.
| @@ -102,7 +106,13 @@ class ObjectParser::Impl { | |||
| "': (code=", static_cast<int>(str_result.error()), ")"); | |||
| } | |||
| map.emplace(std::string(key), std::string(str_result.value())); | |||
| std::string_view str; | |||
| if (auto error = std::move(str_result).get(str)) { | |||
| return Status::Invalid("Error getting value for key '", std::string(key), | |||
| "': ", simdjson::error_message(error)); | |||
| } | |||
| map.emplace(std::string(key), std::string(str)); | |||
There was a problem hiding this comment.
auto field has type simdjson_result<sj::field>. If we first resolve this to a sj::field, we do not need duplicated value checks for the unescaped_key and value.
Also, first asserting the value (L110), then type-checking the value (L98), is cleaner IMO. (It also works the other way around because simdjson provides overloads for many methods on its result type)
Note that for asserting the value and type-checking the value#50653 adds helper methods we might want to reuse
| bool value; | ||
| if (auto error = std::move(bool_result).get(value)) { | ||
| return Status::Invalid("Error getting bool for key '", key, | ||
| "': ", simdjson::error_message(error)); | ||
| } |
There was a problem hiding this comment.
#50653 adds
template <typename SimdjsonValueType>
Result<SimdjsonValueType> GetJsonAs(sj::value& value) {
that already returns arrow::Results instead of error codes. This might simplify these type-casts
kou
commented
Jul 30, 2026
We should rebase on main before we merge this... This broke our CI... |
taepper
commented
Jul 30, 2026
See #50732 |
### Rationale for this change The simdjson API has a throwing and non-throwing subset. #50672 activated a compiler flag that disables the throwing subset of the API, which broke some CI builds. ### 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? No * GitHub Issue: #50730 Authored-by: Alexander Taepper <alexander.taepper@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit bb0cf77. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
This change enables building Arrow with
SIMDJSON_EXCEPTIONS=0by updating the JSON object parser to use simdjson's non-throwing API.What changes are included?
SIMDJSON_EXCEPTIONS=0for the bundled simdjson dependency.simdjson_result::value()inObjectParserwith the non-throwingget()API.Statusvalues on simdjson errors.Are these changes tested?
Yes.
Fixes: #50654
simdjson#50654