taepper commented on code in PR #50672:
URL: https://github.com/apache/arrow/pull/50672#discussion_r3667373947
##########
cpp/src/arrow/json/object_parser.cc:
##########
@@ -132,7 +142,13 @@ class ObjectParser::Impl {
"': ",
simdjson::error_message(bool_result.error()));
}
- return bool_result.value();
+ bool value;
+ if (auto error = std::move(bool_result).get(value)) {
+ return Status::Invalid("Error getting bool for key '", key,
+ "': ", simdjson::error_message(error));
+ }
Review Comment:
#50653 adds
```
template <typename SimdjsonValueType>
Result<SimdjsonValueType> GetJsonAs(sj::value& value) {
```
that already returns `arrow::Result`s instead of error codes. This might
simplify these type-casts
##########
cpp/src/arrow/json/object_parser.cc:
##########
@@ -69,7 +71,12 @@ class ObjectParser::Impl {
"': ",
simdjson::error_message(str_result.error()));
}
- return std::string(str_result.value());
+ 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);
Review Comment:
#50653 adds
```
template <typename SimdjsonValueType>
Result<SimdjsonValueType> GetJsonAs(sj::value& value) {
```
that already returns `arrow::Result`s instead of error codes. This might
simplify these type-casts
##########
cpp/src/arrow/json/object_parser.cc:
##########
@@ -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));
Review 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
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
