taepper commented on code in PR #50725:
URL: https://github.com/apache/arrow/pull/50725#discussion_r3691373199
##########
cpp/src/arrow/json/from_string.cc:
##########
@@ -155,17 +156,6 @@ Result<SimdjsonValueType> GetJsonAs(sj::value& value) {
return typed_value;
}
Review Comment:
While not used in your changes, it might make sense to keep all helper
functions together and also move this to `util/simdjson_internal.h`
##########
cpp/src/arrow/json/from_string.cc:
##########
Review Comment:
While not used in your changes, it might make sense to keep all helper
functions together and also move this to `util/simdjson_internal.h`
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -99,140 +100,98 @@ void JsonWriter::Double(double value) {
}
Status JsonWriter::WriteValue(sj::value value) {
- sj::json_type type;
- if (auto error = value.type().get(type); error != simdjson::SUCCESS) {
- return Status::Invalid(simdjson::error_message(error));
- }
+ return internal::VisitJsonValue(
+ value,
Review Comment:
Very nice! I think this reads very cleanly!
##########
cpp/src/arrow/json/from_string.cc:
##########
Review Comment:
While not used in your changes, it might make sense to keep all helper
functions together and also move this to `util/simdjson_internal.h`
##########
cpp/src/arrow/json/json_writer_internal.cc:
##########
@@ -99,140 +100,98 @@ void JsonWriter::Double(double value) {
}
Status JsonWriter::WriteValue(sj::value value) {
- sj::json_type type;
- if (auto error = value.type().get(type); error != simdjson::SUCCESS) {
- return Status::Invalid(simdjson::error_message(error));
- }
+ return internal::VisitJsonValue(
+ value,
- switch (type) {
- case sj::json_type::object: {
- StartObject();
+ [&](sj::object object) -> Status {
+ StartObject();
- sj::object object;
- if (auto error = value.get_object().get(object); error !=
simdjson::SUCCESS) {
- return Status::Invalid(simdjson::error_message(error));
- }
+ for (auto field : object) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto key, internal::GetSimdjsonResult(field.unescaped_key(),
+ "Failed to get object key:
"));
- for (auto field : object) {
- std::string_view key;
- if (auto error = field.unescaped_key().get(key); error !=
simdjson::SUCCESS) {
- return Status::Invalid(simdjson::error_message(error));
- }
+ Key(key);
- Key(key);
+ ARROW_ASSIGN_OR_RAISE(
+ auto field_value,
+ internal::GetSimdjsonResult(field.value(), "Failed to get object
value: "));
- sj::value field_value;
- if (auto error = field.value().get(field_value); error !=
simdjson::SUCCESS) {
- return Status::Invalid(simdjson::error_message(error));
+ RETURN_NOT_OK(WriteValue(field_value));
}
- RETURN_NOT_OK(WriteValue(field_value));
- }
-
- EndObject();
- break;
- }
+ EndObject();
+ return Status::OK();
+ },
- case sj::json_type::array: {
- StartArray();
+ [&](sj::array array) -> Status {
+ StartArray();
- sj::array array;
- if (auto error = value.get_array().get(array); error !=
simdjson::SUCCESS) {
- return Status::Invalid(simdjson::error_message(error));
- }
+ for (auto element : array) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto element_value,
+ internal::GetSimdjsonResult(element, "Failed to iterate JSON
array: "));
- for (auto element : array) {
- sj::value element_value;
- if (auto error = element.get(element_value); error !=
simdjson::SUCCESS) {
- return Status::Invalid(simdjson::error_message(error));
+ RETURN_NOT_OK(WriteValue(element_value));
}
- RETURN_NOT_OK(WriteValue(element_value));
- }
+ EndArray();
+ return Status::OK();
+ },
+
+ [&](std::string_view string_value) -> Status {
+ String(string_value);
+ return Status::OK();
+ },
+
+ [&](bool bool_value) -> Status {
+ Bool(bool_value);
+ return Status::OK();
+ },
+
+ [&]() -> Status {
+ Null();
+ return Status::OK();
+ },
+
+ [&](sj::value number_value) -> Status {
Review Comment:
Shouldn't this be a `sj::number` instead? Then we should be left with no
results within the lambda body?
--
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]
