Skip to content

GH-55598: [C++][Parquet] Align Parquet-to-GeoArrow conversions with Parquet, GeoParquet, and arrow-rs #55599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
paleolimbot wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GH-55598: [C++][Parquet] Align Parquet-to-GeoArrow conversions with Parquet, GeoParquet, and arrow-rs #55599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions cpp/src/parquet/arrow/arrow_schema_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1279,16 +1279,13 @@ TEST_F(TestConvertParquetSchema, ParquetSchemaGeoArrowExtensions) {
props.set_arrow_extensions_enabled(true);
props.set_binary_type(binary_type->id());
auto arrow_schema = ::arrow::schema(
{::arrow::field(
"geometry",
test::geoarrow_wkb(R"({"crs": "OGC:CRS84", "crs_type": "authority_code"})",
binary_type),
true),
{::arrow::field("geometry",
test::geoarrow_wkb(R"({"crs": "OGC:CRS84"})", binary_type),
true),
::arrow::field(
"geography",
test::geoarrow_wkb(
R"({"crs": "OGC:CRS84", "crs_type": "authority_code", "edges": "spherical"})",
binary_type),
test::geoarrow_wkb(R"({"crs": "OGC:CRS84", "edges": "spherical"})",
binary_type),
true)});
std::shared_ptr<KeyValueMetadata> metadata{};
ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
Expand Down Expand Up @@ -1582,10 +1579,9 @@ TEST_F(TestConvertArrowSchema, ParquetGeoArrowCrsLonLat) {
ParquetType::BYTE_ARRAY));

// There are several ways that longitude/latitude could be specified when coming from
// GeoArrow, which allows null, missing, arbitrary strings (e.g., Authority:Code), and
// PROJJSON.
// GeoArrow, which allows arbitrary strings (e.g., Authority:Code) and PROJJSON.
std::vector<std::string> geoarrow_lonlat = {
"null", R"("OGC:CRS84")", R"("EPSG:4326")",
R"("OGC:CRS84")", R"("EPSG:4326")",
// Purely the parts of the PROJJSON that we inspect to check the lon/lat case
R"({"id": {"authority": "OGC", "code": "CRS84"}})",
R"({"id": {"authority": "EPSG", "code": "4326"}})",
Expand All @@ -1607,6 +1603,23 @@ TEST_F(TestConvertArrowSchema, ParquetGeoArrowCrsLonLat) {
}
}

TEST_F(TestConvertArrowSchema, ParquetGeoArrowCrsUnknown) {
::arrow::ExtensionTypeGuard guard(test::geoarrow_wkb());

std::vector<std::shared_ptr<Field>> arrow_fields = {
::arrow::field("geometry", test::geoarrow_wkb(R"({"crs": null})"), true),
::arrow::field("geography", test::geoarrow_wkb(R"({"edges": "spherical"})"), true)};

ASSERT_OK(ConvertSchema(arrow_fields));

std::vector<NodePtr> parquet_fields = {
PrimitiveNode::Make("geometry", Repetition::OPTIONAL,
LogicalType::Geometry("srid:0"), ParquetType::BYTE_ARRAY),
PrimitiveNode::Make("geography", Repetition::OPTIONAL,
LogicalType::Geography("srid:0"), ParquetType::BYTE_ARRAY)};
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(parquet_fields));
}

TEST_F(TestConvertArrowSchema, ParquetGeoArrowCrsSrid) {
// Checks that it is possible to write the srid:xxxx reccomendation from GeoArrow
::arrow::ExtensionTypeGuard guard(test::geoarrow_wkb());
Expand Down
38 changes: 23 additions & 15 deletions cpp/src/parquet/geospatial/util_json_internal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,9 @@ ::arrow::Result<std::string> GeospatialGeoArrowCrsToParquetCrs(

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 "";
// GeoArrow uses an omitted CRS to mean that the CRS is unknown. Parquet
// represents this explicitly as the special value "srid:0".
return "srid:0";
}

return json_crs_result.status();
Expand All @@ -54,7 +53,7 @@ ::arrow::Result<std::string> GeospatialGeoArrowCrsToParquetCrs(

ARROW_ASSIGN_OR_RAISE(bool is_null, ::arrow::internal::IsJsonNull(json_crs));
if (is_null) {
return "";
return "srid:0";
}

auto crs_string_result = ::arrow::internal::GetJsonAs<std::string_view>(json_crs);
Expand Down Expand Up @@ -164,7 +163,9 @@ ::arrow::Result<std::string> MakeGeoArrowCrsMetadata(
// of the writer, we do interpret these values according to the suggestion in
// the format and pass on this information to GeoArrow.
if (crs.empty()) {
return R"("crs": "OGC:CRS84", "crs_type": "authority_code")";
return R"("crs": "OGC:CRS84")";
} else if (crs == "srid:0") {
return "";
} else if (crs.starts_with(kSridPrefix)) {
return R"("crs": ")" + std::string(crs.substr(kSridPrefix.size())) +
R"(", "crs_type": "srid")";
Expand Down Expand Up @@ -210,10 +211,9 @@ ::arrow::Result<std::string> EscapeCrsAsJsonIfRequired(std::string_view crs) {

::arrow::Result<std::shared_ptr<const parquet::LogicalType>>
LogicalTypeFromGeoArrowMetadata(std::string_view serialized_data) {
// Parquet has no way to interpret a null or missing CRS, so we choose the most likely
// intent here (that the user meant to use the default Parquet CRS)
// Parquet represents GeoArrow's unknown (omitted) CRS as "srid:0".
if (serialized_data.empty() || serialized_data == "{}") {
return LogicalType::Geometry();
return LogicalType::Geometry("srid:0");
}

simdjson::ondemand::parser parser;
Expand Down Expand Up @@ -247,10 +247,15 @@ LogicalTypeFromGeoArrowMetadata(std::string_view serialized_data) {
return LogicalType::Geometry(crs);
}

if (edges_string == "spherical") {
return LogicalType::Geography(crs,
LogicalType::EdgeInterpolationAlgorithm::SPHERICAL);
}
using EdgeAlgorithm = LogicalType::EdgeInterpolationAlgorithm;
if (edges_string == "spherical")
return LogicalType::Geography(crs, EdgeAlgorithm::SPHERICAL);
if (edges_string == "vincenty")
return LogicalType::Geography(crs, EdgeAlgorithm::VINCENTY);
if (edges_string == "thomas") return LogicalType::Geography(crs, EdgeAlgorithm::THOMAS);
if (edges_string == "andoyer")
return LogicalType::Geography(crs, EdgeAlgorithm::ANDOYER);
if (edges_string == "karney") return LogicalType::Geography(crs, EdgeAlgorithm::KARNEY);

return ::arrow::Status::Invalid("Unsupported GeoArrow edge type: ", serialized_data);
}
Expand Down Expand Up @@ -281,8 +286,11 @@ ::arrow::Result<std::shared_ptr<::arrow::DataType>> GeoArrowTypeFromLogicalType(
MakeGeoArrowCrsMetadata(geospatial_type.crs(), metadata));
std::string edges_metadata =
R"("edges": ")" + std::string(geospatial_type.algorithm_name()) + R"(")";
std::string serialized_data =
std::string("{") + crs_metadata + ", " + edges_metadata + "}";
std::string serialized_data = std::string("{") + crs_metadata;
if (!crs_metadata.empty()) {
serialized_data += ", ";
}
serialized_data += edges_metadata + "}";
return maybe_geoarrow_wkb->Deserialize(storage_type, serialized_data);
} else {
throw ParquetException("Can't export logical type ", logical_type.ToString(),
Expand Down
119 changes: 119 additions & 0 deletions cpp/src/parquet/geospatial/util_json_internal_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "parquet/geospatial/util_json_internal.h"

#include <memory>
#include <string>

#include <gtest/gtest.h>

Expand All @@ -30,6 +31,124 @@

namespace parquet {

struct LogicalTypeToGeoArrowCase {
std::shared_ptr<const LogicalType> logical_type;
std::string expected_metadata;
};

class LogicalTypeToGeoArrowTest
: public ::testing::TestWithParam<LogicalTypeToGeoArrowCase> {};

TEST_P(LogicalTypeToGeoArrowTest, ConvertsMetadata) {
::arrow::ExtensionTypeGuard guard(test::geoarrow_wkb());
const auto& test_case = GetParam();
ARROW_SCOPED_TRACE("logical type = ", test_case.logical_type->ToString());

ASSERT_OK_AND_ASSIGN(
auto type,
GeoArrowTypeFromLogicalType(*test_case.logical_type, nullptr, ::arrow::binary()));
auto extension = std::dynamic_pointer_cast<::arrow::ExtensionType>(type);
ASSERT_NE(extension, nullptr);
ASSERT_OK_AND_ASSIGN(auto actual,
::arrow::internal::MinifyJson(extension->Serialize()));
EXPECT_EQ(actual, test_case.expected_metadata);
}

INSTANTIATE_TEST_SUITE_P(
GeoArrowConversion, LogicalTypeToGeoArrowTest,
::testing::Values(
LogicalTypeToGeoArrowCase{LogicalType::Geometry(), R"({"crs":"OGC:CRS84"})"},
LogicalTypeToGeoArrowCase{LogicalType::Geometry("srid:0"), R"({})"},
LogicalTypeToGeoArrowCase{LogicalType::Geometry("EPSG:4267"),
R"({"crs":"EPSG:4267"})"},
LogicalTypeToGeoArrowCase{
LogicalType::Geometry(R"({"id":{"authority":"EPSG","code":4326}})"),
R"({"crs":{"id":{"authority":"EPSG","code":4326}}})"},
LogicalTypeToGeoArrowCase{LogicalType::Geography(),
R"({"crs":"OGC:CRS84","edges":"spherical"})"},
LogicalTypeToGeoArrowCase{
LogicalType::Geography("",
LogicalType::EdgeInterpolationAlgorithm::SPHERICAL),
R"({"crs":"OGC:CRS84","edges":"spherical"})"},
LogicalTypeToGeoArrowCase{
LogicalType::Geography("", LogicalType::EdgeInterpolationAlgorithm::KARNEY),
R"({"crs":"OGC:CRS84","edges":"karney"})"},
LogicalTypeToGeoArrowCase{
LogicalType::Geography("", LogicalType::EdgeInterpolationAlgorithm::VINCENTY),
R"({"crs":"OGC:CRS84","edges":"vincenty"})"},
LogicalTypeToGeoArrowCase{
LogicalType::Geography("", LogicalType::EdgeInterpolationAlgorithm::ANDOYER),
R"({"crs":"OGC:CRS84","edges":"andoyer"})"},
LogicalTypeToGeoArrowCase{
LogicalType::Geography("", LogicalType::EdgeInterpolationAlgorithm::THOMAS),
R"({"crs":"OGC:CRS84","edges":"thomas"})"},
LogicalTypeToGeoArrowCase{LogicalType::Geography("srid:0"),
R"({"edges":"spherical"})"},
LogicalTypeToGeoArrowCase{LogicalType::Geography("EPSG:4267"),
R"({"crs":"EPSG:4267","edges":"spherical"})"},
LogicalTypeToGeoArrowCase{
LogicalType::Geography(R"({"id":{"authority":"EPSG","code":4326}})"),
R"({"crs":{"id":{"authority":"EPSG","code":4326}},"edges":"spherical"})"}));

struct GeoArrowToLogicalTypeCase {
std::string metadata;
std::shared_ptr<const LogicalType> expected_logical_type;
};

class GeoArrowToLogicalTypeTest
: public ::testing::TestWithParam<GeoArrowToLogicalTypeCase> {};

TEST_P(GeoArrowToLogicalTypeTest, ConvertsMetadata) {
const auto& test_case = GetParam();
ARROW_SCOPED_TRACE("metadata = ", test_case.metadata);
ASSERT_OK_AND_ASSIGN(auto actual, LogicalTypeFromGeoArrowMetadata(test_case.metadata));
EXPECT_TRUE(actual->Equals(*test_case.expected_logical_type))
<< "Actual: " << actual->ToString()
<< ", expected: " << test_case.expected_logical_type->ToString();
}

INSTANTIATE_TEST_SUITE_P(
GeoArrowConversion, GeoArrowToLogicalTypeTest,
::testing::Values(
GeoArrowToLogicalTypeCase{R"({})", LogicalType::Geometry("srid:0")},
GeoArrowToLogicalTypeCase{R"({"crs":"EPSG:4267"})",
LogicalType::Geometry("EPSG:4267")},
GeoArrowToLogicalTypeCase{
R"({"crs":{"id":{"authority":"EPSG","code":3857}}})",
LogicalType::Geometry(R"({"id":{"authority":"EPSG","code":3857}})")},
GeoArrowToLogicalTypeCase{R"({"crs":"OGC:CRS84"})", LogicalType::Geometry()},
GeoArrowToLogicalTypeCase{R"({"crs":"EPSG:4326"})", LogicalType::Geometry()},
GeoArrowToLogicalTypeCase{R"({"crs":{"id":{"authority":"EPSG","code":4326}}})",
LogicalType::Geometry()},
GeoArrowToLogicalTypeCase{R"({"crs":{"id":{"authority":"EPSG","code":"4326"}}})",
LogicalType::Geometry()},
GeoArrowToLogicalTypeCase{R"({"crs":{"id":{"authority":"OGC","code":"CRS84"}}})",
LogicalType::Geometry()},
GeoArrowToLogicalTypeCase{R"({"edges":"spherical"})",
LogicalType::Geography("srid:0")},
GeoArrowToLogicalTypeCase{R"({"crs":"OGC:CRS84","edges":"spherical"})",
LogicalType::Geography()},
GeoArrowToLogicalTypeCase{
R"({"crs":"OGC:CRS84","edges":"karney"})",
LogicalType::Geography("", LogicalType::EdgeInterpolationAlgorithm::KARNEY)},
GeoArrowToLogicalTypeCase{
R"({"crs":"OGC:CRS84","edges":"vincenty"})",
LogicalType::Geography("",
LogicalType::EdgeInterpolationAlgorithm::VINCENTY)},
GeoArrowToLogicalTypeCase{
R"({"crs":"OGC:CRS84","edges":"andoyer"})",
LogicalType::Geography("", LogicalType::EdgeInterpolationAlgorithm::ANDOYER)},
GeoArrowToLogicalTypeCase{
R"({"crs":"OGC:CRS84","edges":"thomas"})",
LogicalType::Geography("", LogicalType::EdgeInterpolationAlgorithm::THOMAS)},
GeoArrowToLogicalTypeCase{
R"({"crs":"EPSG:4267","edges":"karney"})",
LogicalType::Geography("EPSG:4267",
LogicalType::EdgeInterpolationAlgorithm::KARNEY)},
GeoArrowToLogicalTypeCase{
R"({"crs":{"id":{"authority":"EPSG","code":4267}},"edges":"spherical"})",
LogicalType::Geography(R"({"id":{"authority":"EPSG","code":4267}})")}));

TEST(UtilJsonInternal, InvalidProjJsonIsEscaped) {
::arrow::ExtensionTypeGuard guard(test::geoarrow_wkb());

Expand Down
2 changes: 1 addition & 1 deletion cpp/src/parquet/test_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ std::shared_ptr<::arrow::DataType> geoarrow_wkb_lonlat(
const std::shared_ptr<::arrow::DataType> storage) {
// There are other ways to express lon/lat output, but this is the one that will
// roundtrip into Parquet and back
return geoarrow_wkb(R"({"crs": "OGC:CRS84", "crs_type": "authority_code"})", storage);
return geoarrow_wkb(R"({"crs": "OGC:CRS84"})", storage);
}

} // namespace test
Expand Down
Loading