From 0230946fb2297542e61d8aac2fe9acd1a1c74b93 Mon Sep 17 00:00:00 2001 From: Aaditya Srinivasan Date: Mon, 3 Aug 2026 15:54:12 +0530 Subject: [PATCH 1/4] Replace remaining RapidJSON usage with simdjson --- cpp/src/parquet/CMakeLists.txt | 2 +- .../parquet/geospatial/util_json_internal.cc | 150 ++++++++++++------ cpp/src/parquet/meson.build | 2 +- cpp/src/parquet/reader_test.cc | 24 ++- cpp/src/parquet/schema_test.cc | 6 +- cpp/src/parquet/types.cc | 15 +- 6 files changed, 119 insertions(+), 80 deletions(-) diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt index e5860d891964..ebc5e16a1590 100644 --- a/cpp/src/parquet/CMakeLists.txt +++ b/cpp/src/parquet/CMakeLists.txt @@ -323,7 +323,7 @@ if(ARROW_TESTING) # "link" our dependencies so that include paths are configured # correctly target_link_libraries(parquet_testing PUBLIC ${ARROW_GTEST_GMOCK}) - list(APPEND PARQUET_TEST_LINK_LIBS parquet_testing RapidJSON) + list(APPEND PARQUET_TEST_LINK_LIBS parquet_testing simdjson::simdjson) endif() if(NOT ARROW_BUILD_SHARED) diff --git a/cpp/src/parquet/geospatial/util_json_internal.cc b/cpp/src/parquet/geospatial/util_json_internal.cc index 6278ab8873c8..7052f73e9512 100644 --- a/cpp/src/parquet/geospatial/util_json_internal.cc +++ b/cpp/src/parquet/geospatial/util_json_internal.cc @@ -20,13 +20,11 @@ #include #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 -#include - #include "parquet/exception.h" #include "parquet/types.h" @@ -34,36 +32,72 @@ namespace parquet { namespace { ::arrow::Result 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()) { + } + + if (auto string = ::arrow::internal::GetJsonAs(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 ""; + } + } else if (auto crs_object = + ::arrow::internal::GetJsonAs(json_crs); + crs_object.ok()) { // 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") { - return ""; - } else if (identifier["authority"] == "EPSG" && identifier["code"].IsInt() && - identifier["code"].GetInt() == 4326) { - return ""; + 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(authority)); + + auto code_string = ::arrow::internal::GetJsonAs(code); + + if (code_string.ok()) { + if ((authority_string == "OGC" && *code_string == "CRS84") || + (authority_string == "EPSG" && *code_string == "4326")) { + return ""; + } + } else if (authority_string == "EPSG") { + auto code_int = ::arrow::internal::GetJsonAs(code); + + if (code_int.ok() && *code_int == 4326) { + return ""; + } } } } @@ -71,14 +105,14 @@ ::arrow::Result 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 writer(buffer); - json_crs.Accept(writer); - return buffer.GetString(); + auto string = ::arrow::internal::GetJsonAs(json_crs); + if (string.ok()) { + return std::string(*string); } + + ::arrow::json::JsonWriter writer; + RETURN_NOT_OK(writer.WriteValue(json_crs)); + return std::string(writer.GetString().ValueUnsafe()); } // Utility for ensuring that a Parquet CRS is valid JSON when written to @@ -125,18 +159,18 @@ ::arrow::Result 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 writer(buffer); - rj::Value v; - v.SetString(crs.data(), static_cast(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(); + return std::string(escaped); } + + return std::string(crs); } } // namespace @@ -149,24 +183,42 @@ ::arrow::Result> 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) { return ::arrow::Status::Invalid("Invalid serialized JSON data: ", serialized_data); } - ARROW_ASSIGN_OR_RAISE(std::string crs, GeospatialGeoArrowCrsToParquetCrs(document)); + ARROW_ASSIGN_OR_RAISE( + auto object, ::arrow::internal::GetSimdjsonResult(document.get_object(), + "Failed to get JSON object: ")); + + ARROW_ASSIGN_OR_RAISE(std::string crs, GeospatialGeoArrowCrsToParquetCrs(object)); + + auto edges_field = object["edges"]; + + if (edges_field.error() == simdjson::NO_SUCH_FIELD) { + return LogicalType::Geometry(crs); + } + + ARROW_ASSIGN_OR_RAISE(auto edges, ::arrow::internal::GetSimdjsonResult( + edges_field, "Failed to get 'edges' field: ")); - if (document.HasMember("edges") && document["edges"] == "planar") { + ARROW_ASSIGN_OR_RAISE(auto edges_string, + ::arrow::internal::GetJsonAs(edges)); + + if (edges_string == "planar") { return LogicalType::Geometry(crs); - } else if (document.HasMember("edges") && document["edges"] == "spherical") { + } + + if (edges_string == "spherical") { return LogicalType::Geography(crs, LogicalType::EdgeInterpolationAlgorithm::SPHERICAL); - } else if (document.HasMember("edges")) { - return ::arrow::Status::Invalid("Unsupported GeoArrow edge type: ", serialized_data); } - return LogicalType::Geometry(crs); + return ::arrow::Status::Invalid("Unsupported GeoArrow edge type: ", serialized_data); } ::arrow::Result> GeoArrowTypeFromLogicalType( diff --git a/cpp/src/parquet/meson.build b/cpp/src/parquet/meson.build index 9069ccb5fd1a..6add6e450573 100644 --- a/cpp/src/parquet/meson.build +++ b/cpp/src/parquet/meson.build @@ -88,7 +88,7 @@ if not thrift_dep.found() thrift_dep = thrift_proj.dependency('thrift') endif -parquet_deps = [arrow_dep, rapidjson_dep, thrift_dep] +parquet_deps = [arrow_dep, thrift_dep] if needs_parquet_encryption or get_option('parquet_require_encryption').auto() openssl_dep = dependency('openssl', required: needs_parquet_encryption) diff --git a/cpp/src/parquet/reader_test.cc b/cpp/src/parquet/reader_test.cc index 7ae9021e35e9..6fdbcb159725 100644 --- a/cpp/src/parquet/reader_test.cc +++ b/cpp/src/parquet/reader_test.cc @@ -27,12 +27,6 @@ #include #include -#include "arrow/json/rapidjson_defs.h" // IWYU pragma: keep - -#include -#include -#include - #include "arrow/array.h" #include "arrow/array/array_binary.h" #include "arrow/array/builder_binary.h" @@ -44,6 +38,7 @@ #include "arrow/util/checked_cast.h" #include "arrow/util/config.h" #include "arrow/util/range.h" +#include "arrow/util/simdjson_internal.h" #include "parquet/column_reader.h" #include "parquet/column_scanner.h" @@ -59,8 +54,6 @@ #include "parquet/test_util.h" #include "parquet/types.h" -namespace rj = arrow::rapidjson; - using arrow::internal::checked_pointer_cast; using arrow::internal::Zip; @@ -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(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)) { + return ::arrow::Status::Invalid("JSON parse error: ", simdjson::error_message(error)); } + return ::arrow::Status::OK(); } diff --git a/cpp/src/parquet/schema_test.cc b/cpp/src/parquet/schema_test.cc index 859f14a34d91..704b2da79c1b 100644 --- a/cpp/src/parquet/schema_test.cc +++ b/cpp/src/parquet/schema_test.cc @@ -1581,9 +1581,9 @@ TEST(TestLogicalTypeOperation, LogicalTypeRepresentation) { {LogicalType::Geometry(R"(crs with "quotes" and \backslashes\)"), R"(Geometry(crs=crs with "quotes" and \backslashes\))", R"({"Type": "Geometry", "crs": "crs with \"quotes\" and \\backslashes\\"})"}, - {LogicalType::Geometry("crs with control characters \u0001 and \u001F"), - "Geometry(crs=crs with control characters \u0001 and \u001F)", - R"({"Type": "Geometry", "crs": "crs with control characters \u0001 and \u001F"})"}, + {LogicalType::Geometry("crs with control characters \u0001 and \u001f"), + "Geometry(crs=crs with control characters \u0001 and \u001f)", + R"({"Type": "Geometry", "crs": "crs with control characters \u0001 and \u001f"})"}, {LogicalType::Geography(), "Geography(crs=, algorithm=spherical)", R"({"Type": "Geography"})"}, {LogicalType::Geography("srid:1234", diff --git a/cpp/src/parquet/types.cc b/cpp/src/parquet/types.cc index 9d7604faec30..cc3199f367af 100644 --- a/cpp/src/parquet/types.cc +++ b/cpp/src/parquet/types.cc @@ -24,16 +24,13 @@ #include #include -#include "arrow/json/rapidjson_defs.h" // IWYU pragma: keep +#include "arrow/json/json_writer_internal.h" #include "arrow/util/checked_cast.h" #include "arrow/util/compression.h" #include "arrow/util/decimal.h" #include "arrow/util/float16.h" #include "arrow/util/logging_internal.h" -#include -#include - #include "parquet/exception.h" #include "parquet/thrift_internal.h" #include "parquet/types.h" @@ -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 writer(buffer); - rj::Value v; - v.SetString(crs.data(), static_cast(crs.size())); - v.Accept(writer); - json << R"(, "crs": )" << buffer.GetString(); + ::arrow::json::JsonWriter writer; + writer.String(crs); + json << R"(, "crs": )" << writer.GetString().ValueUnsafe(); } } // namespace From 777999e80bbae38a4656f7536e28825ab9140355 Mon Sep 17 00:00:00 2001 From: Aaditya Srinivasan Date: Tue, 4 Aug 2026 10:33:28 +0530 Subject: [PATCH 2/4] Preserve GeoArrow CRS JSON formatting --- .../parquet/geospatial/util_json_internal.cc | 113 ++++++++++-------- 1 file changed, 66 insertions(+), 47 deletions(-) diff --git a/cpp/src/parquet/geospatial/util_json_internal.cc b/cpp/src/parquet/geospatial/util_json_internal.cc index 7052f73e9512..6305d82ded71 100644 --- a/cpp/src/parquet/geospatial/util_json_internal.cc +++ b/cpp/src/parquet/geospatial/util_json_internal.cc @@ -17,6 +17,7 @@ #include "parquet/geospatial/util_json_internal.h" +#include #include #include "arrow/extension_type.h" @@ -57,47 +58,53 @@ ::arrow::Result GeospatialGeoArrowCrsToParquetCrs( // longitude/latitude in WGS84 according to the Parquet specification return ""; } - } else if (auto crs_object = - ::arrow::internal::GetJsonAs(json_crs); - crs_object.ok()) { - // 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(authority)); - - auto code_string = ::arrow::internal::GetJsonAs(code); - - if (code_string.ok()) { - if ((authority_string == "OGC" && *code_string == "CRS84") || - (authority_string == "EPSG" && *code_string == "4326")) { - return ""; - } - } else if (authority_string == "EPSG") { - auto code_int = ::arrow::internal::GetJsonAs(code); - - if (code_int.ok() && *code_int == 4326) { - 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(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(authority)); + + auto code_string = ::arrow::internal::GetJsonAs(code); + + if (code_string.ok()) { + if ((authority_string == "OGC" && *code_string == "CRS84") || + (authority_string == "EPSG" && *code_string == "4326")) { + return ""; + } + } else if (authority_string == "EPSG") { + auto code_int = ::arrow::internal::GetJsonAs(code); + + if (code_int.ok() && *code_int == 4326) { + return ""; } } } @@ -105,14 +112,26 @@ ::arrow::Result 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) - auto string = ::arrow::internal::GetJsonAs(json_crs); - if (string.ok()) { - return std::string(*string); + RETURN_NOT_OK(::arrow::internal::GetSimdjsonResult(crs_object.reset(), + "Failed to reset 'crs' object: ") + .status()); + + ARROW_ASSIGN_OR_RAISE(auto raw_crs, + ::arrow::internal::GetSimdjsonResult( + crs_object.raw_json(), "Failed to get raw 'crs' JSON: ")); + + std::string minified(raw_crs.size(), '\0'); + size_t minified_len = 0; + + if (auto error = + simdjson::minify(raw_crs.data(), raw_crs.size(), minified.data(), minified_len); + error != simdjson::SUCCESS) { + return ::arrow::Status::Invalid("Failed to minify CRS JSON: ", + simdjson::error_message(error)); } - ::arrow::json::JsonWriter writer; - RETURN_NOT_OK(writer.WriteValue(json_crs)); - return std::string(writer.GetString().ValueUnsafe()); + minified.resize(minified_len); + return minified; } // Utility for ensuring that a Parquet CRS is valid JSON when written to From 3ccd7465660f9d590233f4ae2a4a56ee58bf5ec8 Mon Sep 17 00:00:00 2001 From: Aaditya Srinivasan Date: Wed, 5 Aug 2026 09:47:29 +0530 Subject: [PATCH 3/4] Address feedback --- cpp/src/arrow/json/from_string.cc | 10 +-- cpp/src/arrow/json/json_writer_internal.cc | 18 +++--- cpp/src/arrow/util/simdjson_internal.h | 34 +++++----- cpp/src/parquet/CMakeLists.txt | 6 +- .../parquet/geospatial/util_json_internal.cc | 63 ++++++++++--------- 5 files changed, 70 insertions(+), 61 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index c9d910667133..f573ae710651 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -106,7 +106,7 @@ class ConcreteConverter : public JSONConverter { int32_t num_elements = 0; for (auto element : json_array) { ARROW_ASSIGN_OR_RAISE(auto value, - internal::GetSimdjsonResult( + internal::ResolveSimdjsonResult( element, "Could not iterate elements of JSON array: ")); RETURN_NOT_OK(self->AppendValue(value)); num_elements++; @@ -287,7 +287,7 @@ Status ProcessJsonArrayElements( } ARROW_ASSIGN_OR_RAISE(sj::value element, - internal::GetSimdjsonResult( + internal::ResolveSimdjsonResult( *it, "Could not iterate elements of JSON array: ")); RETURN_NOT_OK(handler(element)); ++it; @@ -652,7 +652,7 @@ class MapConverter final : public ConcreteConverter { for (auto json_pair_result : array) { ARROW_ASSIGN_OR_RAISE( auto json_pair, - internal::GetSimdjsonResult( + internal::ResolveSimdjsonResult( json_pair_result, "Could not iterate elements of JSON array: ")); ARROW_ASSIGN_OR_RAISE(auto json_pair_array, internal::GetJsonAs(json_pair)); @@ -763,7 +763,7 @@ class StructConverter final : public ConcreteConverter { size_t i = 0; for (auto child : array) { ARROW_ASSIGN_OR_RAISE(auto child_value, - internal::GetSimdjsonResult( + internal::ResolveSimdjsonResult( child, "Could not iterate elements of JSON array: ")); RETURN_NOT_OK(child_converters_[i]->AppendValue(child_value)); ++i; @@ -779,7 +779,7 @@ class StructConverter final : public ConcreteConverter { std::vector field_seen(num_fields, false); for (auto field_result : object) { ARROW_ASSIGN_OR_RAISE(auto field, - internal::GetSimdjsonResult( + internal::ResolveSimdjsonResult( field_result, "Error getting field of object: ")); std::string_view key; if (field.unescaped_key(/*allow_replacement=*/false).get(key) != diff --git a/cpp/src/arrow/json/json_writer_internal.cc b/cpp/src/arrow/json/json_writer_internal.cc index 567694902497..85028b233377 100644 --- a/cpp/src/arrow/json/json_writer_internal.cc +++ b/cpp/src/arrow/json/json_writer_internal.cc @@ -108,14 +108,14 @@ Status JsonWriter::WriteValue(sj::value value) { for (auto field : object) { ARROW_ASSIGN_OR_RAISE( - auto key, internal::GetSimdjsonResult(field.unescaped_key(), - "Failed to get object key: ")); + auto key, internal::ResolveSimdjsonResult(field.unescaped_key(), + "Failed to get object key: ")); Key(key); - ARROW_ASSIGN_OR_RAISE( - auto field_value, - internal::GetSimdjsonResult(field.value(), "Failed to get object value: ")); + ARROW_ASSIGN_OR_RAISE(auto field_value, + internal::ResolveSimdjsonResult( + field.value(), "Failed to get object value: ")); RETURN_NOT_OK(WriteValue(field_value)); } @@ -130,7 +130,7 @@ Status JsonWriter::WriteValue(sj::value value) { for (auto element : array) { ARROW_ASSIGN_OR_RAISE( auto element_value, - internal::GetSimdjsonResult(element, "Failed to iterate JSON array: ")); + internal::ResolveSimdjsonResult(element, "Failed to iterate JSON array: ")); RETURN_NOT_OK(WriteValue(element_value)); } @@ -170,9 +170,9 @@ Status JsonWriter::WriteValue(sj::value value) { }, [&](sj::value value) -> Status { - ARROW_ASSIGN_OR_RAISE(auto raw_json, - internal::GetSimdjsonResult(simdjson::to_json_string(value), - "Failed to get raw JSON: ")); + ARROW_ASSIGN_OR_RAISE(auto raw_json, internal::ResolveSimdjsonResult( + simdjson::to_json_string(value), + "Failed to get raw JSON: ")); RawValue(raw_json); return Status::OK(); }); diff --git a/cpp/src/arrow/util/simdjson_internal.h b/cpp/src/arrow/util/simdjson_internal.h index 8ffb741da42e..04b14d9b13a9 100644 --- a/cpp/src/arrow/util/simdjson_internal.h +++ b/cpp/src/arrow/util/simdjson_internal.h @@ -82,10 +82,11 @@ constexpr const char* JsonTypeName() { } template -Result GetSimdjsonResult(simdjson::simdjson_result result, std::string_view error) { +Result ResolveSimdjsonResult(simdjson::simdjson_result 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; } @@ -98,33 +99,34 @@ Status VisitJsonValue(simdjson::ondemand::value value, ObjectFn&& object_fn, NullFn&& null_fn, Int64Fn&& int64_fn, Uint64Fn&& uint64_fn, DoubleFn&& double_fn, BigIntegerFn&& big_integer_fn) { ARROW_ASSIGN_OR_RAISE( - auto type, GetSimdjsonResult(value.type(), "Failed to determine JSON type: ")); + auto type, ResolveSimdjsonResult(value.type(), "Failed to determine JSON type: ")); switch (type) { case simdjson::ondemand::json_type::object: { ARROW_ASSIGN_OR_RAISE( auto object, - GetSimdjsonResult(value.get_object(), "Failed to get JSON object: ")); + ResolveSimdjsonResult(value.get_object(), "Failed to get JSON object: ")); return object_fn(object); } case simdjson::ondemand::json_type::array: { ARROW_ASSIGN_OR_RAISE( - auto array, GetSimdjsonResult(value.get_array(), "Failed to get JSON array: ")); + auto array, + ResolveSimdjsonResult(value.get_array(), "Failed to get JSON array: ")); return array_fn(array); } case simdjson::ondemand::json_type::string: { ARROW_ASSIGN_OR_RAISE( auto string, - GetSimdjsonResult(value.get_string(), "Failed to get JSON string: ")); + ResolveSimdjsonResult(value.get_string(), "Failed to get JSON string: ")); return string_fn(string); } case simdjson::ondemand::json_type::boolean: { ARROW_ASSIGN_OR_RAISE( auto boolean, - GetSimdjsonResult(value.get_bool(), "Failed to get JSON boolean: ")); + ResolveSimdjsonResult(value.get_bool(), "Failed to get JSON boolean: ")); return bool_fn(boolean); } @@ -132,29 +134,31 @@ Status VisitJsonValue(simdjson::ondemand::value value, ObjectFn&& object_fn, return null_fn(); case simdjson::ondemand::json_type::number: { - ARROW_ASSIGN_OR_RAISE(auto number_type, - GetSimdjsonResult(value.get_number_type(), - "Failed to determine JSON number type: ")); + ARROW_ASSIGN_OR_RAISE( + auto number_type, + ResolveSimdjsonResult(value.get_number_type(), + "Failed to determine JSON number type: ")); switch (number_type) { case simdjson::ondemand::number_type::signed_integer: { ARROW_ASSIGN_OR_RAISE( auto number, - GetSimdjsonResult(value.get_int64(), "Failed to get signed integer: ")); + ResolveSimdjsonResult(value.get_int64(), "Failed to get signed integer: ")); return int64_fn(number); } case simdjson::ondemand::number_type::unsigned_integer: { ARROW_ASSIGN_OR_RAISE( - auto number, - GetSimdjsonResult(value.get_uint64(), "Failed to get unsigned integer: ")); + auto number, ResolveSimdjsonResult(value.get_uint64(), + "Failed to get unsigned integer: ")); return uint64_fn(number); } case simdjson::ondemand::number_type::floating_point_number: { ARROW_ASSIGN_OR_RAISE( - auto number, GetSimdjsonResult(value.get_double(), - "Failed to get floating-point number: ")); + auto number, + ResolveSimdjsonResult(value.get_double(), + "Failed to get floating-point number: ")); return double_fn(number); } diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt index ebc5e16a1590..212414ad8033 100644 --- a/cpp/src/parquet/CMakeLists.txt +++ b/cpp/src/parquet/CMakeLists.txt @@ -263,9 +263,9 @@ endif() list(APPEND PARQUET_SHARED_LINK_LIBS arrow_shared) -# Add RapidJSON & simdjson libraries -list(APPEND PARQUET_SHARED_PRIVATE_LINK_LIBS RapidJSON simdjson::simdjson) -list(APPEND PARQUET_STATIC_LINK_LIBS RapidJSON simdjson::simdjson) +# Add simdjson libraries +list(APPEND PARQUET_SHARED_PRIVATE_LINK_LIBS simdjson::simdjson) +list(APPEND PARQUET_STATIC_LINK_LIBS simdjson::simdjson) # These are libraries that we will link privately with parquet_shared (as they # do not need to be linked transitively by other linkers) diff --git a/cpp/src/parquet/geospatial/util_json_internal.cc b/cpp/src/parquet/geospatial/util_json_internal.cc index 6305d82ded71..236d0584a013 100644 --- a/cpp/src/parquet/geospatial/util_json_internal.cc +++ b/cpp/src/parquet/geospatial/util_json_internal.cc @@ -43,7 +43,7 @@ ::arrow::Result GeospatialGeoArrowCrsToParquetCrs( return ""; } - ARROW_ASSIGN_OR_RAISE(auto json_crs, ::arrow::internal::GetSimdjsonResult( + 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)); @@ -51,17 +51,18 @@ ::arrow::Result GeospatialGeoArrowCrsToParquetCrs( return ""; } - if (auto string = ::arrow::internal::GetJsonAs(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 + auto crs_string_result = ::arrow::internal::GetJsonAs(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(*string); + return std::string(crs_string); } ARROW_ASSIGN_OR_RAISE( @@ -75,7 +76,7 @@ ::arrow::Result GeospatialGeoArrowCrsToParquetCrs( auto id_field = crs_object["id"]; if (id_field.error() != simdjson::NO_SUCH_FIELD) { - ARROW_ASSIGN_OR_RAISE(auto identifier, ::arrow::internal::GetSimdjsonResult( + ARROW_ASSIGN_OR_RAISE(auto identifier, ::arrow::internal::ResolveSimdjsonResult( id_field, "Failed to get 'id' field: ")); auto authority_field = identifier["authority"]; @@ -84,26 +85,28 @@ ::arrow::Result GeospatialGeoArrowCrsToParquetCrs( if (authority_field.error() != simdjson::NO_SUCH_FIELD && code_field.error() != simdjson::NO_SUCH_FIELD) { ARROW_ASSIGN_OR_RAISE(auto authority, - ::arrow::internal::GetSimdjsonResult( + ::arrow::internal::ResolveSimdjsonResult( authority_field, "Failed to get 'authority' field: ")); - ARROW_ASSIGN_OR_RAISE(auto code, ::arrow::internal::GetSimdjsonResult( + ARROW_ASSIGN_OR_RAISE(auto code, ::arrow::internal::ResolveSimdjsonResult( code_field, "Failed to get 'code' field: ")); ARROW_ASSIGN_OR_RAISE(auto authority_string, ::arrow::internal::GetJsonAs(authority)); - auto code_string = ::arrow::internal::GetJsonAs(code); + auto code_string_result = ::arrow::internal::GetJsonAs(code); + + if (code_string_result.ok()) { + auto code_string = *code_string_result; - if (code_string.ok()) { - if ((authority_string == "OGC" && *code_string == "CRS84") || - (authority_string == "EPSG" && *code_string == "4326")) { + if ((authority_string == "OGC" && code_string == "CRS84") || + (authority_string == "EPSG" && code_string == "4326")) { return ""; } } else if (authority_string == "EPSG") { - auto code_int = ::arrow::internal::GetJsonAs(code); + auto code_int_result = ::arrow::internal::GetJsonAs(code); - if (code_int.ok() && *code_int == 4326) { + if (code_int_result.ok() && *code_int_result == 4326) { return ""; } } @@ -112,12 +115,12 @@ ::arrow::Result 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) - RETURN_NOT_OK(::arrow::internal::GetSimdjsonResult(crs_object.reset(), - "Failed to reset 'crs' object: ") + RETURN_NOT_OK(::arrow::internal::ResolveSimdjsonResult(crs_object.reset(), + "Failed to reset 'crs' object: ") .status()); ARROW_ASSIGN_OR_RAISE(auto raw_crs, - ::arrow::internal::GetSimdjsonResult( + ::arrow::internal::ResolveSimdjsonResult( crs_object.raw_json(), "Failed to get raw 'crs' JSON: ")); std::string minified(raw_crs.size(), '\0'); @@ -137,7 +140,7 @@ ::arrow::Result GeospatialGeoArrowCrsToParquetCrs( // Utility for ensuring that a Parquet CRS is valid JSON when written to // GeoArrow metadata (without escaping it if it is already valid JSON such as // a PROJJSON string) -std::string EscapeCrsAsJsonIfRequired(std::string_view crs); +::arrow::Result EscapeCrsAsJsonIfRequired(std::string_view crs); ::arrow::Result MakeGeoArrowCrsMetadata( std::string_view crs, @@ -166,18 +169,20 @@ ::arrow::Result 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) { +::arrow::Result EscapeCrsAsJsonIfRequired(std::string_view crs) { simdjson::ondemand::parser parser; simdjson::padded_string json(crs); @@ -185,7 +190,7 @@ std::string EscapeCrsAsJsonIfRequired(std::string_view crs) { ::arrow::json::JsonWriter writer; writer.String(crs); - auto escaped = writer.GetString().ValueUnsafe(); + ARROW_ASSIGN_OR_RAISE(auto escaped, writer.GetString()); return std::string(escaped); } @@ -210,9 +215,9 @@ ::arrow::Result> LogicalTypeFromGeoArrowMetad return ::arrow::Status::Invalid("Invalid serialized JSON data: ", serialized_data); } - ARROW_ASSIGN_OR_RAISE( - auto object, ::arrow::internal::GetSimdjsonResult(document.get_object(), - "Failed to get JSON object: ")); + ARROW_ASSIGN_OR_RAISE(auto object, + ::arrow::internal::ResolveSimdjsonResult( + document.get_object(), "Failed to get JSON object: ")); ARROW_ASSIGN_OR_RAISE(std::string crs, GeospatialGeoArrowCrsToParquetCrs(object)); @@ -222,7 +227,7 @@ ::arrow::Result> LogicalTypeFromGeoArrowMetad return LogicalType::Geometry(crs); } - ARROW_ASSIGN_OR_RAISE(auto edges, ::arrow::internal::GetSimdjsonResult( + ARROW_ASSIGN_OR_RAISE(auto edges, ::arrow::internal::ResolveSimdjsonResult( edges_field, "Failed to get 'edges' field: ")); ARROW_ASSIGN_OR_RAISE(auto edges_string, From 487d90f3555811764feb23247c7bd57fe1786171 Mon Sep 17 00:00:00 2001 From: Aaditya Srinivasan Date: Wed, 5 Aug 2026 16:47:26 +0530 Subject: [PATCH 4/4] Address feedback 2 --- cpp/src/arrow/json/json_writer_internal.cc | 8 +- cpp/src/arrow/util/simdjson_internal.h | 110 ++++++++++++++-- cpp/src/parquet/CMakeLists.txt | 1 + .../parquet/geospatial/util_json_internal.cc | 122 +++++++++--------- .../geospatial/util_json_internal_test.cc | 66 ++++++++++ cpp/src/parquet/meson.build | 1 + cpp/src/parquet/reader_test.cc | 6 +- 7 files changed, 230 insertions(+), 84 deletions(-) create mode 100644 cpp/src/parquet/geospatial/util_json_internal_test.cc diff --git a/cpp/src/arrow/json/json_writer_internal.cc b/cpp/src/arrow/json/json_writer_internal.cc index 85028b233377..6d3e9ff9d85d 100644 --- a/cpp/src/arrow/json/json_writer_internal.cc +++ b/cpp/src/arrow/json/json_writer_internal.cc @@ -109,13 +109,13 @@ Status JsonWriter::WriteValue(sj::value value) { for (auto field : object) { ARROW_ASSIGN_OR_RAISE( auto key, internal::ResolveSimdjsonResult(field.unescaped_key(), - "Failed to get object key: ")); + "Failed to get object key")); Key(key); ARROW_ASSIGN_OR_RAISE(auto field_value, internal::ResolveSimdjsonResult( - field.value(), "Failed to get object value: ")); + field.value(), "Failed to get object value")); RETURN_NOT_OK(WriteValue(field_value)); } @@ -130,7 +130,7 @@ Status JsonWriter::WriteValue(sj::value value) { for (auto element : array) { ARROW_ASSIGN_OR_RAISE( auto element_value, - internal::ResolveSimdjsonResult(element, "Failed to iterate JSON array: ")); + internal::ResolveSimdjsonResult(element, "Failed to iterate JSON array")); RETURN_NOT_OK(WriteValue(element_value)); } @@ -172,7 +172,7 @@ Status JsonWriter::WriteValue(sj::value value) { [&](sj::value value) -> Status { ARROW_ASSIGN_OR_RAISE(auto raw_json, internal::ResolveSimdjsonResult( simdjson::to_json_string(value), - "Failed to get raw JSON: ")); + "Failed to get raw JSON")); RawValue(raw_json); return Status::OK(); }); diff --git a/cpp/src/arrow/util/simdjson_internal.h b/cpp/src/arrow/util/simdjson_internal.h index 04b14d9b13a9..1badd99938b1 100644 --- a/cpp/src/arrow/util/simdjson_internal.h +++ b/cpp/src/arrow/util/simdjson_internal.h @@ -99,34 +99,34 @@ Status VisitJsonValue(simdjson::ondemand::value value, ObjectFn&& object_fn, NullFn&& null_fn, Int64Fn&& int64_fn, Uint64Fn&& uint64_fn, DoubleFn&& double_fn, BigIntegerFn&& big_integer_fn) { ARROW_ASSIGN_OR_RAISE( - auto type, ResolveSimdjsonResult(value.type(), "Failed to determine JSON type: ")); + auto type, ResolveSimdjsonResult(value.type(), "Failed to determine JSON type")); switch (type) { case simdjson::ondemand::json_type::object: { ARROW_ASSIGN_OR_RAISE( auto object, - ResolveSimdjsonResult(value.get_object(), "Failed to get JSON object: ")); + ResolveSimdjsonResult(value.get_object(), "Failed to get JSON object")); return object_fn(object); } case simdjson::ondemand::json_type::array: { ARROW_ASSIGN_OR_RAISE( auto array, - ResolveSimdjsonResult(value.get_array(), "Failed to get JSON array: ")); + ResolveSimdjsonResult(value.get_array(), "Failed to get JSON array")); return array_fn(array); } case simdjson::ondemand::json_type::string: { ARROW_ASSIGN_OR_RAISE( auto string, - ResolveSimdjsonResult(value.get_string(), "Failed to get JSON string: ")); + ResolveSimdjsonResult(value.get_string(), "Failed to get JSON string")); return string_fn(string); } case simdjson::ondemand::json_type::boolean: { ARROW_ASSIGN_OR_RAISE( auto boolean, - ResolveSimdjsonResult(value.get_bool(), "Failed to get JSON boolean: ")); + ResolveSimdjsonResult(value.get_bool(), "Failed to get JSON boolean")); return bool_fn(boolean); } @@ -137,28 +137,27 @@ Status VisitJsonValue(simdjson::ondemand::value value, ObjectFn&& object_fn, ARROW_ASSIGN_OR_RAISE( auto number_type, ResolveSimdjsonResult(value.get_number_type(), - "Failed to determine JSON number type: ")); + "Failed to determine JSON number type")); switch (number_type) { case simdjson::ondemand::number_type::signed_integer: { ARROW_ASSIGN_OR_RAISE( auto number, - ResolveSimdjsonResult(value.get_int64(), "Failed to get signed integer: ")); + ResolveSimdjsonResult(value.get_int64(), "Failed to get signed integer")); return int64_fn(number); } case simdjson::ondemand::number_type::unsigned_integer: { - ARROW_ASSIGN_OR_RAISE( - auto number, ResolveSimdjsonResult(value.get_uint64(), - "Failed to get unsigned integer: ")); + ARROW_ASSIGN_OR_RAISE(auto number, + ResolveSimdjsonResult(value.get_uint64(), + "Failed to get unsigned integer")); return uint64_fn(number); } case simdjson::ondemand::number_type::floating_point_number: { ARROW_ASSIGN_OR_RAISE( - auto number, - ResolveSimdjsonResult(value.get_double(), - "Failed to get floating-point number: ")); + auto number, ResolveSimdjsonResult(value.get_double(), + "Failed to get floating-point number")); return double_fn(number); } @@ -244,5 +243,90 @@ Result GetJsonAs(simdjson::ondemand::value& value) { return typed_value; } +template +Result GetJsonField(simdjson::ondemand::object& object, std::string_view key) { + for (auto field_result : object) { + ARROW_ASSIGN_OR_RAISE( + auto field, ResolveSimdjsonResult(field_result, "Failed to iterate JSON object")); + + ARROW_ASSIGN_OR_RAISE( + auto field_key, + ResolveSimdjsonResult(field.unescaped_key(), "Failed to get JSON object key")); + + if (field_key == key) { + auto value = field.value(); + + if constexpr (std::is_same_v) { + return value; + } else { + return GetJsonAs(value); + } + } + } + + return Status::KeyError("Missing JSON field: ", key); +} + +inline Result MinifyJson(std::string_view json) { + std::string minified(json.size(), '\0'); + size_t minified_len = 0; + + if (auto error = + simdjson::minify(json.data(), json.size(), minified.data(), minified_len); + error != simdjson::SUCCESS) { + return Status::Invalid("Failed to minify JSON: ", simdjson::error_message(error)); + } + + minified.resize(minified_len); + return minified; +} + +inline Status ValidateJsonObject(simdjson::ondemand::object object); + +inline Status ValidateJsonArray(simdjson::ondemand::array array); + +inline Status ConsumeJsonValue(simdjson::ondemand::value value) { + return VisitJsonValue( + value, ValidateJsonObject, ValidateJsonArray, + [](std::string_view) { return Status::OK(); }, [](bool) { return Status::OK(); }, + []() { return Status::OK(); }, [](int64_t) { return Status::OK(); }, + [](uint64_t) { return Status::OK(); }, [](double) { return Status::OK(); }, + [](simdjson::ondemand::value) { return Status::OK(); }); +} + +inline Status ValidateJsonObject(simdjson::ondemand::object object) { + for (auto field_result : object) { + ARROW_ASSIGN_OR_RAISE( + auto field, ResolveSimdjsonResult(field_result, "Failed to iterate JSON object")); + + RETURN_NOT_OK(ConsumeJsonValue(field.value())); + } + + return Status::OK(); +} + +inline Status ValidateJsonArray(simdjson::ondemand::array array) { + for (auto element_result : array) { + ARROW_ASSIGN_OR_RAISE( + auto value, + ResolveSimdjsonResult(element_result, "Failed to iterate JSON array")); + + RETURN_NOT_OK(ConsumeJsonValue(value)); + } + + return Status::OK(); +} + +inline Status ValidateJsonDocument(simdjson::ondemand::parser& parser, + simdjson::padded_string& json) { + ARROW_ASSIGN_OR_RAISE( + auto document, ResolveSimdjsonResult(parser.iterate(json), "Failed to parse JSON")); + + ARROW_ASSIGN_OR_RAISE(auto value, ResolveSimdjsonResult(document.get_value(), + "Failed to get JSON value")); + + return ConsumeJsonValue(value); +} + } // namespace internal } // namespace arrow diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt index 212414ad8033..606dcdc0a9e3 100644 --- a/cpp/src/parquet/CMakeLists.txt +++ b/cpp/src/parquet/CMakeLists.txt @@ -379,6 +379,7 @@ add_parquet_test(internals-test bloom_filter_test.cc geospatial/statistics_test.cc geospatial/util_internal_test.cc + geospatial/util_json_internal_test.cc metadata_test.cc page_index_test.cc properties_test.cc diff --git a/cpp/src/parquet/geospatial/util_json_internal.cc b/cpp/src/parquet/geospatial/util_json_internal.cc index 236d0584a013..0650dda6a73b 100644 --- a/cpp/src/parquet/geospatial/util_json_internal.cc +++ b/cpp/src/parquet/geospatial/util_json_internal.cc @@ -34,17 +34,21 @@ namespace parquet { namespace { ::arrow::Result GeospatialGeoArrowCrsToParquetCrs( simdjson::ondemand::object object) { - auto crs_field = object["crs"]; + auto json_crs_result = + ::arrow::internal::GetJsonField(object, "crs"); + + if (!json_crs_result.ok()) { + if (json_crs_result.status().IsKeyError()) { + // 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 ""; + } - 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 ""; + return json_crs_result.status(); } - ARROW_ASSIGN_OR_RAISE(auto json_crs, ::arrow::internal::ResolveSimdjsonResult( - crs_field, "Failed to get 'crs' field: ")); + auto json_crs = *std::move(json_crs_result); ARROW_ASSIGN_OR_RAISE(bool is_null, ::arrow::internal::IsJsonNull(json_crs)); if (is_null) { @@ -57,6 +61,8 @@ ::arrow::Result GeospatialGeoArrowCrsToParquetCrs( auto crs_string = *crs_string_result; if (crs_string == "EPSG:4326" || crs_string == "OGC:CRS84") { + // crs can be left empty because these cases both correspond to + // longitude/latitude in WGS84 according to the Parquet specification return ""; } @@ -73,41 +79,39 @@ ::arrow::Result GeospatialGeoArrowCrsToParquetCrs( // 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"]; + auto identifier_result = + ::arrow::internal::GetJsonField(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: ")); + if (identifier_result.ok()) { + auto identifier = *std::move(identifier_result); - auto authority_field = identifier["authority"]; - auto code_field = identifier["code"]; + auto authority_result = + ::arrow::internal::GetJsonField(identifier, "authority"); - if (authority_field.error() != simdjson::NO_SUCH_FIELD && - code_field.error() != simdjson::NO_SUCH_FIELD) { - ARROW_ASSIGN_OR_RAISE(auto authority, - ::arrow::internal::ResolveSimdjsonResult( - authority_field, "Failed to get 'authority' field: ")); + if (authority_result.ok()) { + auto authority_string = *authority_result; - ARROW_ASSIGN_OR_RAISE(auto code, ::arrow::internal::ResolveSimdjsonResult( - code_field, "Failed to get 'code' field: ")); + auto code_result = + ::arrow::internal::GetJsonField(identifier, "code"); - ARROW_ASSIGN_OR_RAISE(auto authority_string, - ::arrow::internal::GetJsonAs(authority)); + if (code_result.ok()) { + auto code = *std::move(code_result); - auto code_string_result = ::arrow::internal::GetJsonAs(code); + auto code_string_result = ::arrow::internal::GetJsonAs(code); - if (code_string_result.ok()) { - auto code_string = *code_string_result; + if (code_string_result.ok()) { + auto code_string = *code_string_result; - if ((authority_string == "OGC" && code_string == "CRS84") || - (authority_string == "EPSG" && code_string == "4326")) { - return ""; - } - } else if (authority_string == "EPSG") { - auto code_int_result = ::arrow::internal::GetJsonAs(code); + if ((authority_string == "OGC" && code_string == "CRS84") || + (authority_string == "EPSG" && code_string == "4326")) { + return ""; + } + } else if (authority_string == "EPSG") { + auto code_int_result = ::arrow::internal::GetJsonAs(code); - if (code_int_result.ok() && *code_int_result == 4326) { - return ""; + if (code_int_result.ok() && *code_int_result == 4326) { + return ""; + } } } } @@ -116,25 +120,14 @@ ::arrow::Result 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) RETURN_NOT_OK(::arrow::internal::ResolveSimdjsonResult(crs_object.reset(), - "Failed to reset 'crs' object: ") + "Failed to reset 'crs' object") .status()); ARROW_ASSIGN_OR_RAISE(auto raw_crs, ::arrow::internal::ResolveSimdjsonResult( - crs_object.raw_json(), "Failed to get raw 'crs' JSON: ")); + crs_object.raw_json(), "Failed to get raw 'crs' JSON")); - std::string minified(raw_crs.size(), '\0'); - size_t minified_len = 0; - - if (auto error = - simdjson::minify(raw_crs.data(), raw_crs.size(), minified.data(), minified_len); - error != simdjson::SUCCESS) { - return ::arrow::Status::Invalid("Failed to minify CRS JSON: ", - simdjson::error_message(error)); - } - - minified.resize(minified_len); - return minified; + return ::arrow::internal::MinifyJson(raw_crs); } // Utility for ensuring that a Parquet CRS is valid JSON when written to @@ -182,16 +175,20 @@ ::arrow::Result MakeGeoArrowCrsMetadata( return R"("crs": )" + escaped; } +::arrow::Result EscapeJsonString(std::string_view value) { + ::arrow::json::JsonWriter writer; + writer.String(value); + + ARROW_ASSIGN_OR_RAISE(auto escaped, writer.GetString()); + return ::arrow::internal::MinifyJson(escaped); +} + ::arrow::Result EscapeCrsAsJsonIfRequired(std::string_view crs) { simdjson::ondemand::parser parser; simdjson::padded_string json(crs); - if (parser.iterate(json).error() != simdjson::SUCCESS) { - ::arrow::json::JsonWriter writer; - writer.String(crs); - - ARROW_ASSIGN_OR_RAISE(auto escaped, writer.GetString()); - return std::string(escaped); + if (!::arrow::internal::ValidateJsonDocument(parser, json).ok()) { + return EscapeJsonString(crs); } return std::string(crs); @@ -210,14 +207,15 @@ ::arrow::Result> LogicalTypeFromGeoArrowMetad 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) { - return ::arrow::Status::Invalid("Invalid serialized JSON data: ", serialized_data); - } + RETURN_NOT_OK(::arrow::internal::ValidateJsonDocument(parser, json)); - ARROW_ASSIGN_OR_RAISE(auto object, - ::arrow::internal::ResolveSimdjsonResult( - document.get_object(), "Failed to get JSON object: ")); + // Reparse because validation consumes the On-Demand document. + ARROW_ASSIGN_OR_RAISE(auto document, ::arrow::internal::ResolveSimdjsonResult( + parser.iterate(json), "Failed to parse JSON")); + + ARROW_ASSIGN_OR_RAISE( + auto object, ::arrow::internal::ResolveSimdjsonResult(document.get_object(), + "Failed to get JSON object")); ARROW_ASSIGN_OR_RAISE(std::string crs, GeospatialGeoArrowCrsToParquetCrs(object)); @@ -228,7 +226,7 @@ ::arrow::Result> LogicalTypeFromGeoArrowMetad } ARROW_ASSIGN_OR_RAISE(auto edges, ::arrow::internal::ResolveSimdjsonResult( - edges_field, "Failed to get 'edges' field: ")); + edges_field, "Failed to get 'edges' field")); ARROW_ASSIGN_OR_RAISE(auto edges_string, ::arrow::internal::GetJsonAs(edges)); diff --git a/cpp/src/parquet/geospatial/util_json_internal_test.cc b/cpp/src/parquet/geospatial/util_json_internal_test.cc new file mode 100644 index 000000000000..8100702d6ce3 --- /dev/null +++ b/cpp/src/parquet/geospatial/util_json_internal_test.cc @@ -0,0 +1,66 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "parquet/geospatial/util_json_internal.h" + +#include + +#include + +#include "arrow/testing/extension_type.h" +#include "arrow/testing/gtest_util.h" +#include "arrow/type.h" + +#include "parquet/geospatial/util_json_internal.h" +#include "parquet/test_util.h" + +namespace parquet { + +TEST(UtilJsonInternal, InvalidProjJsonIsEscaped) { + ::arrow::ExtensionTypeGuard guard(test::geoarrow_wkb()); + + auto metadata = ::arrow::key_value_metadata( + {"proj"}, {R"({"a":[1,2,]})"}); // Invalid JSON (trailing comma) + + auto logical_type = LogicalType::Geometry("projjson:proj"); + + ASSERT_OK_AND_ASSIGN( + auto type, GeoArrowTypeFromLogicalType(*logical_type, metadata, ::arrow::binary())); + + auto extension = std::dynamic_pointer_cast<::arrow::ExtensionType>(type); + ASSERT_NE(extension, nullptr); + + EXPECT_EQ(extension->Serialize(), + R"({"crs": "{\"a\":[1,2,]}", "crs_type": "projjson"})"); +} + +TEST(UtilJsonInternal, EscapedCrsKeyIsRecognized) { + std::string metadata = R"({"cr\u0073":"EPSG:3857","crs_type":"authority_code"})"; + + ASSERT_OK_AND_ASSIGN(auto logical_type, LogicalTypeFromGeoArrowMetadata(metadata)); + + ASSERT_EQ(logical_type->ToString(), "Geometry(crs=EPSG:3857)"); +} + +TEST(UtilJsonInternal, InvalidTrailingMetadataIsRejected) { + auto result = LogicalTypeFromGeoArrowMetadata( + R"({"crs":"EPSG:3857","edges":"planar","unused":[1,2,]})"); + + ASSERT_RAISES(Invalid, result); +} + +} // namespace parquet \ No newline at end of file diff --git a/cpp/src/parquet/meson.build b/cpp/src/parquet/meson.build index 6add6e450573..444ec1d09233 100644 --- a/cpp/src/parquet/meson.build +++ b/cpp/src/parquet/meson.build @@ -198,6 +198,7 @@ parquet_tests = { 'encoding_test.cc', 'geospatial/statistics_test.cc', 'geospatial/util_internal_test.cc', + 'geospatial/util_json_internal_test.cc' 'metadata_test.cc', 'page_index_test.cc', 'properties_test.cc', diff --git a/cpp/src/parquet/reader_test.cc b/cpp/src/parquet/reader_test.cc index 6fdbcb159725..cdeee116fbc3 100644 --- a/cpp/src/parquet/reader_test.cc +++ b/cpp/src/parquet/reader_test.cc @@ -1224,13 +1224,9 @@ namespace { ::arrow::Status CheckJsonValid(std::string_view json_string) { 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)) { - return ::arrow::Status::Invalid("JSON parse error: ", simdjson::error_message(error)); - } + RETURN_NOT_OK(::arrow::internal::ValidateJsonDocument(parser, padded_json)); return ::arrow::Status::OK(); }