kou commented on code in PR #50781:
URL: https://github.com/apache/arrow/pull/50781#discussion_r3716274006
##########
cpp/src/parquet/geospatial/util_json_internal.cc:
##########
@@ -71,14 +112,26 @@ ::arrow::Result<std::string>
GeospatialGeoArrowCrsToParquetCrs(
// If we could not detect a longitude/latitude CRS, just write the string to
the
// LogicalType crs (being sure to unescape a JSON string into a regular
string)
- if (json_crs.IsString()) {
- return json_crs.GetString();
- } else {
- rj::StringBuffer buffer;
- rj::Writer<rj::StringBuffer> writer(buffer);
- json_crs.Accept(writer);
- return buffer.GetString();
+ RETURN_NOT_OK(::arrow::internal::GetSimdjsonResult(crs_object.reset(),
+ "Failed to reset 'crs'
object: ")
Review Comment:
It seems that all `GetSimdjsonResult()` messages have `: ` suffix. How about
adding it automatically?
```diff
diff --git a/cpp/src/arrow/util/simdjson_internal.h
b/cpp/src/arrow/util/simdjson_internal.h
index 8ffb741da4..187dd3aaa5 100644
--- a/cpp/src/arrow/util/simdjson_internal.h
+++ b/cpp/src/arrow/util/simdjson_internal.h
@@ -85,7 +85,7 @@ template <typename T>
Result<T> GetSimdjsonResult(simdjson::simdjson_result<T> result,
std::string_view error) {
T value;
if (auto error_code = std::move(result).get(value); error_code !=
simdjson::SUCCESS) {
- return Status::Invalid(error, simdjson::error_message(error_code));
+ return Status::Invalid(error, ": ",
simdjson::error_message(error_code));
}
return value;
}
```
(BTW, `GetSimdjsonResult()` name may be a bit strange because it doesn't
return `simdjson::result<T>`. It gets `arrow::Result<T>` from
`simdjson::result<T>`. `ResolveSimdjsonResult()`, `SimdjsonResultToArrow()` or
something may be better.)
##########
cpp/src/parquet/geospatial/util_json_internal.cc:
##########
@@ -17,52 +17,93 @@
#include "parquet/geospatial/util_json_internal.h"
+#include <simdjson.h>
#include <string>
#include "arrow/extension_type.h"
-#include "arrow/json/rapidjson_defs.h" // IWYU pragma: keep
+#include "arrow/json/json_writer_internal.h"
#include "arrow/result.h"
+#include "arrow/util/simdjson_internal.h"
#include "arrow/util/string.h"
-#include <rapidjson/document.h>
-#include <rapidjson/writer.h>
-
#include "parquet/exception.h"
#include "parquet/types.h"
namespace parquet {
namespace {
::arrow::Result<std::string> GeospatialGeoArrowCrsToParquetCrs(
- const ::arrow::rapidjson::Document& document) {
- namespace rj = ::arrow::rapidjson;
+ simdjson::ondemand::object object) {
+ auto crs_field = object["crs"];
- if (!document.HasMember("crs") || document["crs"].IsNull()) {
+ if (crs_field.error() == simdjson::NO_SUCH_FIELD) {
// Parquet GEOMETRY/GEOGRAPHY do not have a concept of a null/missing
// CRS, but an omitted one is more likely to have meant "lon/lat" than
// a truly unspecified one (i.e., Engineering CRS with arbitrary XY units)
return "";
}
- const auto& json_crs = document["crs"];
- if (json_crs.IsString() && (json_crs == "EPSG:4326" || json_crs ==
"OGC:CRS84")) {
- // crs can be left empty because these cases both correspond to
- // longitude/latitude in WGS84 according to the Parquet specification
+ ARROW_ASSIGN_OR_RAISE(auto json_crs, ::arrow::internal::GetSimdjsonResult(
+ crs_field, "Failed to get 'crs'
field: "));
+
+ ARROW_ASSIGN_OR_RAISE(bool is_null, ::arrow::internal::IsJsonNull(json_crs));
+ if (is_null) {
return "";
- } else if (json_crs.IsObject()) {
- // Attempt to detect common PROJJSON representations of longitude/latitude
and return
- // an empty crs to maximize compatibility with readers that do not
implement CRS
- // support. PROJJSON stores this in the "id" member like:
- // {..., "id": {"authority": "...", "code": "..."}}
- if (json_crs.HasMember("id")) {
- const auto& identifier = json_crs["id"];
- if (identifier.HasMember("authority") && identifier.HasMember("code")) {
- if (identifier["authority"] == "OGC" && identifier["code"] == "CRS84")
{
- return "";
- } else if (identifier["authority"] == "EPSG" && identifier["code"] ==
"4326") {
+ }
+
+ if (auto string = ::arrow::internal::GetJsonAs<std::string_view>(json_crs);
+ string.ok()) {
+ if (*string == "EPSG:4326" || *string == "OGC:CRS84") {
+ // crs can be left empty because these cases both correspond to
+ // longitude/latitude in WGS84 according to the Parquet specification
+ return "";
+ }
+
+ // If we could not detect a longitude/latitude CRS, just write the string
to the
+ // LogicalType crs (being sure to unescape a JSON string into a regular
string)
+ return std::string(*string);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(
+ auto crs_object,
+ ::arrow::internal::GetJsonAs<simdjson::ondemand::object>(json_crs));
+
+ // Attempt to detect common PROJJSON representations of longitude/latitude
and return
+ // an empty crs to maximize compatibility with readers that do not implement
CRS
+ // support. PROJJSON stores this in the "id" member like:
+ // {..., "id": {"authority": "...", "code": "..."}}
+ auto id_field = crs_object["id"];
+
+ if (id_field.error() != simdjson::NO_SUCH_FIELD) {
+ ARROW_ASSIGN_OR_RAISE(auto identifier,
::arrow::internal::GetSimdjsonResult(
+ id_field, "Failed to get 'id'
field: "));
+
+ auto authority_field = identifier["authority"];
+ auto code_field = identifier["code"];
+
+ if (authority_field.error() != simdjson::NO_SUCH_FIELD &&
+ code_field.error() != simdjson::NO_SUCH_FIELD) {
+ ARROW_ASSIGN_OR_RAISE(auto authority,
+ ::arrow::internal::GetSimdjsonResult(
+ authority_field, "Failed to get 'authority'
field: "));
+
+ ARROW_ASSIGN_OR_RAISE(auto code, ::arrow::internal::GetSimdjsonResult(
+ code_field, "Failed to get 'code'
field: "));
+
+ ARROW_ASSIGN_OR_RAISE(auto authority_string,
+
::arrow::internal::GetJsonAs<std::string_view>(authority));
+
+ auto code_string = ::arrow::internal::GetJsonAs<std::string_view>(code);
+
+ if (code_string.ok()) {
+ if ((authority_string == "OGC" && *code_string == "CRS84") ||
+ (authority_string == "EPSG" && *code_string == "4326")) {
Review Comment:
How about using `_result` suffix for `arrow::Result` variable for
readability?
```suggestion
auto code_string_result =
::arrow::internal::GetJsonAs<std::string_view>(code);
if (code_string_result.ok()) {
auto code_string = *code_string_result;
if ((authority_string == "OGC" && code_string == "CRS84") ||
(authority_string == "EPSG" && code_string == "4326")) {
```
##########
cpp/src/parquet/geospatial/util_json_internal.cc:
##########
@@ -17,52 +17,93 @@
#include "parquet/geospatial/util_json_internal.h"
+#include <simdjson.h>
#include <string>
#include "arrow/extension_type.h"
-#include "arrow/json/rapidjson_defs.h" // IWYU pragma: keep
+#include "arrow/json/json_writer_internal.h"
#include "arrow/result.h"
+#include "arrow/util/simdjson_internal.h"
#include "arrow/util/string.h"
-#include <rapidjson/document.h>
-#include <rapidjson/writer.h>
-
#include "parquet/exception.h"
#include "parquet/types.h"
namespace parquet {
namespace {
::arrow::Result<std::string> GeospatialGeoArrowCrsToParquetCrs(
- const ::arrow::rapidjson::Document& document) {
- namespace rj = ::arrow::rapidjson;
+ simdjson::ondemand::object object) {
+ auto crs_field = object["crs"];
- if (!document.HasMember("crs") || document["crs"].IsNull()) {
+ if (crs_field.error() == simdjson::NO_SUCH_FIELD) {
// Parquet GEOMETRY/GEOGRAPHY do not have a concept of a null/missing
// CRS, but an omitted one is more likely to have meant "lon/lat" than
// a truly unspecified one (i.e., Engineering CRS with arbitrary XY units)
return "";
}
- const auto& json_crs = document["crs"];
- if (json_crs.IsString() && (json_crs == "EPSG:4326" || json_crs ==
"OGC:CRS84")) {
- // crs can be left empty because these cases both correspond to
- // longitude/latitude in WGS84 according to the Parquet specification
+ ARROW_ASSIGN_OR_RAISE(auto json_crs, ::arrow::internal::GetSimdjsonResult(
+ crs_field, "Failed to get 'crs'
field: "));
+
+ ARROW_ASSIGN_OR_RAISE(bool is_null, ::arrow::internal::IsJsonNull(json_crs));
+ if (is_null) {
return "";
- } else if (json_crs.IsObject()) {
- // Attempt to detect common PROJJSON representations of longitude/latitude
and return
- // an empty crs to maximize compatibility with readers that do not
implement CRS
- // support. PROJJSON stores this in the "id" member like:
- // {..., "id": {"authority": "...", "code": "..."}}
- if (json_crs.HasMember("id")) {
- const auto& identifier = json_crs["id"];
- if (identifier.HasMember("authority") && identifier.HasMember("code")) {
- if (identifier["authority"] == "OGC" && identifier["code"] == "CRS84")
{
- return "";
- } else if (identifier["authority"] == "EPSG" && identifier["code"] ==
"4326") {
+ }
+
+ if (auto string = ::arrow::internal::GetJsonAs<std::string_view>(json_crs);
+ string.ok()) {
+ if (*string == "EPSG:4326" || *string == "OGC:CRS84") {
Review Comment:
Can we avoid `if (...; ...)` for readability?
```suggestion
auto crs_string_result =
::arrow::internal::GetJsonAs<std::string_view>(json_crs);
if (crs_string_result.ok()) {
auto crs_string = *crs_string_result;
if (crs_string == "EPSG:4326" || crs_string == "OGC:CRS84") {
```
##########
cpp/src/parquet/types.cc:
##########
@@ -1785,13 +1782,9 @@ namespace {
void WriteCrsKeyAndValue(const std::string_view crs, std::ostream& json) {
// There is no restriction on the crs value here, and it may contain quotes
// or backslashes that would result in invalid JSON if unescaped.
- namespace rj = ::arrow::rapidjson;
- rj::StringBuffer buffer;
- rj::Writer<rj::StringBuffer> writer(buffer);
- rj::Value v;
- v.SetString(crs.data(), static_cast<int32_t>(crs.size()));
- v.Accept(writer);
- json << R"(, "crs": )" << buffer.GetString();
+ ::arrow::json::JsonWriter writer;
+ writer.String(crs);
+ json << R"(, "crs": )" << writer.GetString().ValueUnsafe();
Review Comment:
Can we use `JsonWriter` for all JSON build instead of mixing manual JSON
build and `JsonWriter` build?
##########
cpp/src/parquet/geospatial/util_json_internal.cc:
##########
@@ -125,18 +178,18 @@ ::arrow::Result<std::string> MakeGeoArrowCrsMetadata(
}
std::string EscapeCrsAsJsonIfRequired(std::string_view crs) {
- namespace rj = ::arrow::rapidjson;
- rj::Document document;
- if (document.Parse(crs.data(), crs.length()).HasParseError()) {
- rj::StringBuffer buffer;
- rj::Writer<rj::StringBuffer> writer(buffer);
- rj::Value v;
- v.SetString(crs.data(), static_cast<int32_t>(crs.size()));
- v.Accept(writer);
- return std::string(buffer.GetString());
- } else {
- return std::string(crs);
+ simdjson::ondemand::parser parser;
+ simdjson::padded_string json(crs);
+
+ if (parser.iterate(json).error() != simdjson::SUCCESS) {
+ ::arrow::json::JsonWriter writer;
+ writer.String(crs);
+
+ auto escaped = writer.GetString().ValueUnsafe();
Review Comment:
Should we change the return type to `arrow::Result<std::string>` from
`std::string` to propagate an 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
