wgtmac commented on code in PR #50781:
URL: https://github.com/apache/arrow/pull/50781#discussion_r3718415940
##########
cpp/src/parquet/geospatial/util_json_internal.cc:
##########
@@ -17,52 +17,96 @@
#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::ResolveSimdjsonResult(
+ 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") {
+ }
+
+ 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") {
+ 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(crs_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::ResolveSimdjsonResult(
+ id_field, "Failed to get 'id'
field: "));
+
+ auto authority_field = identifier["authority"];
Review Comment:
`authority_field` is consumed only after `identifier["code"]` has been
accessed. In simdjson On-Demand, a field value is valid only until the next
field lookup on the same object; development checks may report
`OUT_OF_ORDER_ITERATION`, and using it without those checks is unsafe. Could we
consume and convert `authority` before looking up `code`, and avoid retaining
multiple lazy field results?
##########
cpp/src/parquet/geospatial/util_json_internal.cc:
##########
@@ -113,30 +169,32 @@ ::arrow::Result<std::string> MakeGeoArrowCrsMetadata(
ARROW_ASSIGN_OR_RAISE(std::string projjson_value,
metadata->Get(metadata_field));
// This value should be valid JSON, but if it is not, we escape it as a
string such
// that it can be inspected by the consumer of GeoArrow.
- return R"("crs": )" + EscapeCrsAsJsonIfRequired(projjson_value) +
- R"(, "crs_type": "projjson")";
+ ARROW_ASSIGN_OR_RAISE(auto escaped,
EscapeCrsAsJsonIfRequired(projjson_value));
+ return R"("crs": )" + escaped + R"(, "crs_type": "projjson")";
}
}
// Pass on the string directly to GeoArrow. If the string is already valid
JSON,
// insert it directly into GeoArrow's "crs" field. Otherwise, escape it and
pass it as a
// string value.
- return R"("crs": )" + EscapeCrsAsJsonIfRequired(crs);
+ ARROW_ASSIGN_OR_RAISE(auto escaped, EscapeCrsAsJsonIfRequired(crs));
+
+ return R"("crs": )" + escaped;
}
-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);
+::arrow::Result<std::string> EscapeCrsAsJsonIfRequired(std::string_view crs) {
+ simdjson::ondemand::parser parser;
+ simdjson::padded_string json(crs);
+
+ if (parser.iterate(json).error() != simdjson::SUCCESS) {
Review Comment:
`parser.iterate(json).error()` does not mean that the whole JSON value is
valid. On-Demand validates only values that are consumed, and this document is
never consumed here; `simdjson::minify()` also explicitly does not validate its
input. As a result, malformed CRS content can be returned unescaped and then
embedded as invalid GeoArrow metadata, whereas the RapidJSON implementation
escaped it as a string. Could we fully parse or consume the value before
deciding to return it verbatim?
##########
cpp/src/parquet/geospatial/util_json_internal.cc:
##########
@@ -17,52 +17,96 @@
#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"];
Review Comment:
RapidJSON matched decoded object keys, but simdjson default `object["key"]`
lookup does not unescape keys. Valid metadata such as
`{"cr\u0073":"EPSG:3857"}` will therefore be treated as missing `crs`, and an
escaped `edges` key can silently change GEOGRAPHY to GEOMETRY. Could we
preserve the previous behavior by matching `field.unescaped_key()` or using a
fully parsed DOM object, and add an escaped-key regression test?
##########
cpp/src/parquet/geospatial/util_json_internal.cc:
##########
@@ -149,24 +207,42 @@ ::arrow::Result<std::shared_ptr<const LogicalType>>
LogicalTypeFromGeoArrowMetad
return LogicalType::Geometry();
}
- namespace rj = ::arrow::rapidjson;
- rj::Document document;
- if (document.Parse(serialized_data.data(),
serialized_data.length()).HasParseError()) {
+ simdjson::ondemand::parser parser;
+ simdjson::padded_string json(serialized_data);
+
+ simdjson::ondemand::document document;
+ if (auto error = parser.iterate(json).get(document); error !=
simdjson::SUCCESS) {
Review Comment:
This only consumes the fields needed by the selected branch, so successful
`iterate()` plus `get_object()` no longer guarantees that the full extension
metadata is valid JSON. For example, invalid content in an unvisited field
after `edges` can be accepted even though RapidJSON rejected the document
before conversion. Could we fully validate the metadata before interpreting
`crs` and `edges`?
##########
cpp/src/parquet/reader_test.cc:
##########
@@ -1230,14 +1223,15 @@ TEST_F(TestJSONWithLocalFile, JSONOutputSortColumns) {
namespace {
::arrow::Status CheckJsonValid(std::string_view json_string) {
- rj::Document json_doc;
- constexpr auto kParseFlags = rj::kParseFullPrecisionFlag |
rj::kParseNanAndInfFlag;
- json_doc.Parse<kParseFlags>(json_string.data(), json_string.length());
- if (json_doc.HasParseError()) {
- return ::arrow::Status::Invalid("JSON parse error at offset ",
- json_doc.GetErrorOffset(), ": ",
-
rj::GetParseError_En(json_doc.GetParseError()));
+ simdjson::ondemand::parser parser;
+ simdjson::ondemand::document document;
+
+ auto padded_json = simdjson::padded_string(json_string);
+
+ if (auto error = parser.iterate(padded_json).get(document)) {
Review Comment:
`CheckJsonValid()` no longer validates the full JSON output: it creates an
On-Demand document but never consumes it, so only stage-1 errors are detected.
This can let malformed printer output pass the test. The previous parser also
intentionally enabled full precision and NaN/Inf, so the replacement should
preserve that contract while actually traversing and validating the document.
##########
cpp/src/arrow/util/simdjson_internal.h:
##########
@@ -82,10 +82,11 @@ constexpr const char* JsonTypeName() {
}
template <typename T>
-Result<T> GetSimdjsonResult(simdjson::simdjson_result<T> result,
std::string_view error) {
+Result<T> ResolveSimdjsonResult(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));
Review Comment:
`ResolveSimdjsonResult()` now inserts `": "`, but all callers still pass
prefixes ending in `": "`, so errors become `Failed ...: : <simdjson error>`.
Could we either remove the suffix from every caller or keep delimiter
formatting in the callers only?
--
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]
