WIP: Proof-of-concept Parquet GEOMETRY logical type implementation by paleolimbot · Pull Request #43196 · apache/arrow

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)

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

 #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

+ }
+};
+
+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 <ustcwg@gmail.com>

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!

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

+ 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

+ }
+
+ 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

 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

- 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

+ 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

+ }
+};
+
+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

+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 !

Geometry value writer could make use of the geometry statistics class to populate geometry statistics Geometry column writer now populates correct statistics format/tidy Run clang-tidy Added a test that writes and reads a parquet file containing a geometry column Remove redundant include Fix problems found by reviewers Try to make it build properly on other platforms Address review comments in apache#43196 Resolve compile errors for MSVC Expose getters in GeometryStatistics, Change geometry_types from std::vector<uint32_t> to std::vector<int32_t>, several other minor fixes. Add test case for UpdateSpaced, don't generate min/max stats for geometry columns. Support covering MakeStatistics and Statistics::Make should not be a breaking change ColumnIndex, as well as some other fixes and refacturings Fix compiler warnings on AMD platforms as well as sanitizer warnings Remove all newly added include directives include cmath for std::isnan Test writing WKB encoded geometries using WriteArrow Change the sort order of geometry from unknown to unsigned; resolved several review comments Add generate_covering_ member to be explicit that' we'll generate the coverings from bounding box when populating the encoded statistics Refactor unscoped enums in geometry_util_internal to enum classes Revert more special case handling for unknown sort order Fix WKB covering test to take native endianness into consideration min/max of geometry columns are the WKB representation of lower-left and upper-right points Address latest review comments A better implementation of geometry min/max statistics Update the code to accomodate the latest changes of the standard: 1. Remove metadata property of geometry logical types 2. Remove covering from geometry statistics Fix problem decoding WKB geometries with more than 32 coordinates Re-implemented geometry statistics according to the updated spec: 1. geometry statistics moved out of statistics, it is now a field of column metadata 2. geometry statistics is removed from page index Revert some unnecessary changes

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-actionsBot added awaiting review Awaiting review Component: Parquet Component: C++ labels Jul 9, 2024

paleolimbot force-pushed the parquet-geometry branch from d6c8c70 to 6d3a12dCompare 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 b34b1c2Compare August 12, 2024 19:06

paleolimbotand 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

paleolimbotand 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-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 17, 2024

paleolimbotand 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 <ustcwg@gmail.com>

github-actionsBot 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 and GEOGRAPHY logical types apache/parquet-format#240
Merged

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
Closed

Kontinuation added a commit to Kontinuation/arrow that referenced this pull request Sep 6, 2024
Address review comments inapache#43196
fb134d3

paleolimbot closed this Sep 12, 2024

paleolimbot pushed a commit to paleolimbot/arrow that referenced this pull request Feb 6, 2025
Address review comments inapache#43196
2cef359

Uh oh!

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?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiayuasu commented Aug 31, 2024

Uh oh!

paleolimbot commented Sep 1, 2024

Uh oh!

jiayuasu commented Sep 3, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paleolimbotSep 5, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Sep 5, 2024

Uh oh!

paleolimbot commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

paleolimbotSep 5, 2024•
edited
Loading