Proof-of-concept Parquet GEOMETRY logical type implementation #43977

Kontinuation · 2024-09-06T02:23:14Z

Rationale for this change

This is a continuation of #43196

In apache/parquet-format#240 a GEOMETRY logical type for Parquet is proposed with a proof-of-concept Java implementation ( apache/parquet-java#1379 ). This is a PR to explore what an implementation would look like in C++.

What changes are included in this PR?

We are still in progress of completing all necessary changes to integrate geometry logical type support to the C++ implementation.

[DONE] Adding geometry logical type
[DONE] Adding geometry column statistics
[DONE] Support reading/writing parquet files containing geometry columns

Are these changes tested?

The tests added only cover very basic use cases. Comprehensive tests will be added in future commits.

Are there any user-facing changes?

Yes! (And will eventually be documented)

Co-authored-by: Gang Wu <[email protected]>

Kontinuation · 2024-09-17T00:07:57Z

Please ping me when ready for review again. Thanks!

@wgtmac I have added ColumnIndex and covering support. Please help review this PR when you have time, thank you.

wgtmac

I just finished reviewing it for the 2nd pass. Thanks for the great work!

My main concern is the difference with Java PoC, which generates min/max values in the statistics and page index as if the GEOMETRY column is a pure BYTE_ARRAY column. Otherwise we need to revise the spec to add a lot of exceptions for geometry type. WDYT?

cpp/src/parquet/thrift_internal.h

cpp/src/parquet/column_reader.cc

cpp/src/parquet/CMakeLists.txt

wgtmac · 2024-09-14T01:32:43Z

cpp/src/parquet/types.cc

+  Geometry(std::string crs, LogicalType::GeometryEdges::edges edges,
+           LogicalType::GeometryEncoding::geometry_encoding encoding,
+           std::string metadata)
+      : LogicalType::Impl(LogicalType::Type::GEOMETRY, SortOrder::UNKNOWN),


Suggested change

: LogicalType::Impl(LogicalType::Type::GEOMETRY, SortOrder::UNKNOWN),

: LogicalType::Impl(LogicalType::Type::GEOMETRY, SortOrder::UNSIGNED),

SortOrder::UNSIGNED is the default sort order of BYTE_ARRAY type. Could we just use this so you don't have to change a line in column_writer.cc. The good thing is that ColumnIndex of geometry type can also be generated automatically, though the min/max values are derived from their binary values and useless. This is the same practice used in the Java PoC impl.

This is life-saving. Now all the special handling of geometry statistics for unknown sort order has gone away.

cpp/src/parquet/types.cc

cpp/src/parquet/reader_test.cc

cpp/src/parquet/statistics.cc

wgtmac · 2024-09-18T15:02:31Z

cpp/src/parquet/statistics.cc

+    out.mmax = maxes[3];
+
+    if (coverings_.empty()) {
+      // Generate coverings from bounding box if coverings is not present


When will coverings_ be empty? Is it the default behavior? I'm not sure if we need to check if the edges is planar since bbox is not accurate for spherical edges. BTW, if we don't have a good implementation for coverings, I think we can just ignore it for now.

This is for generating coverings from the bounding box when assembling the encoded representation of the geometry statistics. I've added a member called generate_covering_ to make it more explicit.

wgtmac · 2024-09-18T15:03:44Z

cpp/src/parquet/statistics.h

+
+class GeometryStatisticsImpl;
+
+class PARQUET_EXPORT GeometryStatistics {


Thanks for adding theses!

wgtmac · 2024-09-18T15:07:07Z

cpp/src/parquet/statistics.h

-  return std::static_pointer_cast<TypedStatistics<DType>>(Statistics::Make(
-      descr, encoded_min, encoded_max, num_values, null_count, distinct_count,
-      has_min_max, has_null_count, has_distinct_count, pool));
+    int64_t distinct_count, const EncodedGeometryStatistics& geometry_statistics,


Why not directly add const EncodedGeometryStatistics* geometry_statistics = NULLPTR to the end of the existing function signature?

…several review comments

… coverings from bounding box when populating the encoded statistics

…and upper-right points

Kontinuation · 2024-09-19T13:04:12Z

I just finished reviewing it for the 2nd pass. Thanks for the great work!

My main concern is the difference with Java PoC, which generates min/max values in the statistics and page index as if the GEOMETRY column is a pure BYTE_ARRAY column. Otherwise we need to revise the spec to add a lot of exceptions for geometry type. WDYT?

I've changed the min/max statistics of geometry columns to be the WKB representation of lower-left and upper-right corners in the last commit according to apache/iceberg#10981 (comment).

wgtmac

Thanks for the quick change! I've left some minor comments.

wgtmac · 2024-09-19T15:48:07Z

cpp/src/parquet/column_reader.cc

+    EncodedGeometryStatistics encoded_geometry_stats;
+    if (stats.__isset.geometry_stats) {
+      encoded_geometry_stats = FromThrift(stats.geometry_stats);
+    }
+    page_statistics.set_geometry(encoded_geometry_stats);


Suggested change

EncodedGeometryStatistics encoded_geometry_stats;

if (stats.__isset.geometry_stats) {

encoded_geometry_stats = FromThrift(stats.geometry_stats);

}

page_statistics.set_geometry(encoded_geometry_stats);

page_statistics.set_geometry(FromThrift(stats.geometry_stats));

wgtmac · 2024-09-19T16:08:37Z

cpp/src/parquet/statistics.h

-  return std::static_pointer_cast<TypedStatistics<DType>>(Statistics::Make(
-      descr, encoded_min, encoded_max, num_values, null_count, distinct_count,
-      has_min_max, has_null_count, has_distinct_count, pool));
+    int64_t distinct_count, const EncodedGeometryStatistics& geometry_statistics,


IMO, we'd better not to add another overload. If there is a compelling reason to do so, we can add a ARROW_DEPRECATED macro to the old one.

paleolimbot and others added 30 commits August 12, 2024 14:22

update thrift

4a4ebc9

update thrift defs

6f4e4b7

add stubs

6acb2f2

split methods out of line

6ff5855

maybe to/from thrift

0ac5d84

a few more serializers

8a80ac7

add basic test for serialization

f330997

add sort order check

b34b1c2

update thrift

7e9f9b2

start geom utiles

6a9ac3a

test roundtrip thrift cases

a22908f

more geometry utils

dcc083a

more

9acc840

bounder

e18412c

add basic test

76b3f59

a few more strings

fc77ff2

test some bounding box things

21f11a1

more tests

6bbce5c

fix test

bb41b06

with passing tests

88a42f5

add in WKT equiv

3798ef1

more tests

ccbd616

start on stats

3752f83

implement update/merge for geometry statistics

2ff6078

more complete stats

79e4715

start on factory methods

4444757

more stats things

176c997

maybe work with serde

1de193e

Update cpp/src/parquet/types.cc

d2f8157

Co-authored-by: Gang Wu <[email protected]>

Updated parquet.thrift and re-generated cpp sources

b525e84

Kontinuation added 2 commits September 12, 2024 19:03

ColumnIndex, as well as some other fixes and refacturings

c56133c

Fix compiler warnings on AMD platforms as well as sanitizer warnings

174e1e1

Kontinuation requested a review from wgtmac September 12, 2024 12:30

Kontinuation added 2 commits September 12, 2024 20:36

Remove all newly added include directives

bd0e2ad

include cmath for std::isnan

1521bac

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 12, 2024

paleolimbot mentioned this pull request Sep 12, 2024

WIP: Proof-of-concept Parquet GEOMETRY logical type implementation #43196

Closed

Kontinuation marked this pull request as ready for review September 16, 2024 02:45

Kontinuation changed the title ~~WIP: Proof-of-concept Parquet GEOMETRY logical type implementation~~ Proof-of-concept Parquet GEOMETRY logical type implementation Sep 16, 2024

Test writing WKB encoded geometries using WriteArrow

572e865

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 16, 2024

wgtmac reviewed Sep 18, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 18, 2024

wgtmac mentioned this pull request Sep 18, 2024

PARQUET-2471: Add geometry logical type apache/parquet-format#240

Open

Change the sort order of geometry from unknown to unsigned; resolved …

6c322d5

…several review comments

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 19, 2024

Kontinuation added 5 commits September 19, 2024 15:59

Add generate_covering_ member to be explicit that' we'll generate the…

cd43ba5

… coverings from bounding box when populating the encoded statistics

Refactor unscoped enums in geometry_util_internal to enum classes

33803bc

Revert more special case handling for unknown sort order

31a70c5

Fix WKB covering test to take native endianness into consideration

7dfbf4b

min/max of geometry columns are the WKB representation of lower-left …

5d8ab77

…and upper-right points

wgtmac reviewed Sep 19, 2024

View reviewed changes

Kontinuation added 2 commits September 20, 2024 10:38

Address latest review comments

ae5926f

A better implementation of geometry min/max statistics

672f19c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof-of-concept Parquet GEOMETRY logical type implementation #43977

Proof-of-concept Parquet GEOMETRY logical type implementation #43977

Kontinuation commented Sep 6, 2024 •

edited

Loading

Kontinuation commented Sep 17, 2024

wgtmac left a comment

wgtmac Sep 14, 2024

wgtmac Sep 14, 2024

Kontinuation Sep 19, 2024

wgtmac Sep 18, 2024

Kontinuation Sep 19, 2024

wgtmac Sep 18, 2024

wgtmac Sep 18, 2024

Kontinuation commented Sep 19, 2024

wgtmac left a comment

wgtmac Sep 19, 2024

wgtmac Sep 19, 2024

	: LogicalType::Impl(LogicalType::Type::GEOMETRY, SortOrder::UNKNOWN),
	: LogicalType::Impl(LogicalType::Type::GEOMETRY, SortOrder::UNSIGNED),


		class GeometryStatisticsImpl;

		class PARQUET_EXPORT GeometryStatistics {

Proof-of-concept Parquet GEOMETRY logical type implementation #43977

Are you sure you want to change the base?

Proof-of-concept Parquet GEOMETRY logical type implementation #43977

Conversation

Kontinuation commented Sep 6, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Kontinuation commented Sep 17, 2024

wgtmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kontinuation commented Sep 19, 2024

wgtmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kontinuation commented Sep 6, 2024 •

edited

Loading