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

paleolimbot · 2024-07-09T06:29:46Z

Rationale for this change

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?

Work in progress!

Are these changes tested?

They will be!

Are there any user-facing changes?

Yes! (And will eventually be documented)

cpp/src/parquet/types.h

cpp/src/parquet/types.cc

wgtmac · 2024-08-17T15:21:48Z

cpp/src/parquet/statistics.h

@@ -24,6 +24,7 @@
 #include <string>
 #include <utility>

+#include "parquet/geometry_util.h"


Could we use forward declaration? This is a public header.

wgtmac · 2024-08-17T15:23:47Z

cpp/src/parquet/geometry_util.h

+  }
+};
+
+class WKBBuffer {


Should we move them into arrow/util instead of parquet only?

After thinking twice, I think we can keep this in the parquet module. But it should be renamed to geometry_util_internal.h to avoid making it public.

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

jiayuasu · 2024-08-31T03:01:12Z

@paleolimbot Hey Dewey, I know you have been busy at other projects. We really want to speed up the process of the Parquet Geometry PR since the proposal and Java POC is already done. Do you have an estimated timeline for the C++ POC implementation?

If you have too much on your plate, we will be happy to take over or help with the C++ POC.

paleolimbot · 2024-09-01T16:33:13Z

Sorry for the slow reply, I've been out of office! You are of course welcome to continue on top of this PR...I'm back in office Thursday and can definitely review. I think the main remaining portion is the Arrow IO and serde of the statistics.

jiayuasu · 2024-09-03T17:15:37Z

@paleolimbot Thanks for the message. We will soon create a PR against your branch!

cpp/src/parquet/types.h

wgtmac · 2024-09-05T15:32:01Z

cpp/src/parquet/types.cc

+      crs = type.GEOMETRY.crs;
+    }
+
+    LogicalType::GeometryEdges::edges edges = LogicalType::GeometryEdges::UNKNOWN;


Should we throw when edges is UNKNOWN? It is a required field by design.

I think I was emulating how enum parameters are handled in the date/time types here (but I may not have been!)

addressed in fb134d3

wgtmac · 2024-09-05T15:32:32Z

cpp/src/parquet/types.cc

+    }
+
+    LogicalType::GeometryEncoding::geometry_encoding encoding =
+        LogicalType::GeometryEncoding::UNKNOWN;


ditto, this is a required field by design.

addressed in fb134d3

wgtmac · 2024-09-05T15:46:26Z

cpp/src/parquet/statistics.h

@@ -135,6 +186,7 @@ class PARQUET_EXPORT EncodedStatistics {
  bool has_max = false;
  bool has_null_count = false;
  bool has_distinct_count = false;
+  bool has_geometry_statistics = false;


I know other fields already have has_xxx flags before C++11. Should we use std::optional<EncodedGeometryStatistics> to make it simpler?

My sense would be to handle changing that pattern with a dedicated PR but I have no opinions either way 🙂

wgtmac · 2024-09-05T15:47: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,


Unfortunately this will break downstream users as it is a public API.

Would adding the two new arguments to the end of the argument list (with default values) make this a non-breaking change?

Do we need to make it ABI-compatible or source-compatible? If ABI compatibility is required we must add a new override version of this function.

I think API-compatible is sufficient. Please correct me if I'm wrong @pitrou

wgtmac · 2024-09-05T15:52:49Z

cpp/src/parquet/statistics.cc

+    bounder_.ReadGeometryTypes(other.bounder_.WkbTypes());
+  }
+
+  void Update(const ByteArray* values, int64_t num_values, int64_t null_count) {


We might need a UpdateSpaced variant which deals with inputs with validity bits, like what TypeStatistics::UpdateSpaced does.

I've added UpdateSpaced in fb134d3, will add tests for it later.

wgtmac · 2024-09-05T15:54:39Z

cpp/src/parquet/geometry_util.h

+  }
+};
+
+class WKBBuffer {


After thinking twice, I think we can keep this in the parquet module. But it should be renamed to geometry_util_internal.h to avoid making it public.

wgtmac · 2024-09-05T16:01:55Z

cpp/src/parquet/geometry_util.h

+class WKBGenericSequenceBounder {
+ public:
+  WKBGenericSequenceBounder()
+      : xy_(chunk_),


Why do we need to keep 8 variants?

This is me being fancy and trying to avoid branching 🙂 . A runtime switch is probably fine for a POC and it's possibly worth benchmarking before introducing this complexity.

wgtmac · 2024-09-05T16:06:59Z

Thanks @paleolimbot for initiating the PoC! I've left some inline comments on this PR.

Thanks @jiayuasu and @Kontinuation to continue the effort! Let's work together to make it to the finish line.

paleolimbot · 2024-09-12T21:09:19Z

Closing in favour of #43977 !

paleolimbot requested a review from wgtmac as a code owner July 9, 2024 06:29

paleolimbot marked this pull request as draft July 9, 2024 18:16

github-actions bot added awaiting review Awaiting review Component: Parquet Component: C++ labels Jul 9, 2024

paleolimbot force-pushed the parquet-geometry branch from d6c8c70 to 6d3a12d Compare August 7, 2024 02:15

paleolimbot added 8 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

paleolimbot force-pushed the parquet-geometry branch 3 times, most recently from aa5424d to b34b1c2 Compare August 12, 2024 19:06

paleolimbot and others added 13 commits August 12, 2024 16:21

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

paleolimbot and others added 2 commits August 14, 2024 14:41

more tests

ccbd616

start on stats

3752f83

wgtmac reviewed Aug 17, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 17, 2024

paleolimbot and others added 6 commits August 19, 2024 10:15

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]>

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 19, 2024

wgtmac mentioned this pull request Aug 20, 2024

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

Open

Kontinuation mentioned this pull request Sep 4, 2024

POC Parquet GEOMETRY logical type implementation paleolimbot/arrow#2

Closed

wgtmac reviewed Sep 5, 2024

View reviewed changes

Kontinuation mentioned this pull request Sep 6, 2024

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

Open

Kontinuation added a commit to Kontinuation/arrow that referenced this pull request Sep 6, 2024

Address review comments in apache#43196

fb134d3

paleolimbot closed this Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

paleolimbot commented Jul 9, 2024

wgtmac Aug 17, 2024

wgtmac Aug 17, 2024

wgtmac Sep 5, 2024

jiayuasu commented Aug 31, 2024

paleolimbot commented Sep 1, 2024

jiayuasu commented Sep 3, 2024

wgtmac Sep 5, 2024

paleolimbot Sep 5, 2024

Kontinuation Sep 6, 2024

wgtmac Sep 5, 2024

Kontinuation Sep 6, 2024

wgtmac Sep 5, 2024

paleolimbot Sep 5, 2024 •

edited

Loading

wgtmac Sep 5, 2024

paleolimbot Sep 5, 2024

Kontinuation Sep 6, 2024

wgtmac Sep 6, 2024

wgtmac Sep 5, 2024

Kontinuation Sep 6, 2024

wgtmac Sep 5, 2024

wgtmac Sep 5, 2024

paleolimbot Sep 5, 2024

wgtmac commented Sep 5, 2024

paleolimbot commented Sep 12, 2024

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

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

Conversation

paleolimbot commented Jul 9, 2024

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiayuasu commented Aug 31, 2024

paleolimbot commented Sep 1, 2024

jiayuasu commented Sep 3, 2024

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

paleolimbot Sep 5, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgtmac commented Sep 5, 2024

paleolimbot commented Sep 12, 2024

paleolimbot Sep 5, 2024 •

edited

Loading