PARQUET-2417: Add statistics support to geometry logical type by zhangfengcdt · Pull Request #2971 · apache/parquet-java

zhangfengcdt · 2024-07-26T15:31:30Z

This PR implements the geo types that are introduced in the new parquet specification.
apache/parquet-format@94b9d63

Jira

My PR addresses the following Parquet Jira issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-2471

Tests

My PR adds the following unit tests: TestGeometryTypeRoundTrip

Commits

My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

This PR is copied form this place: #1379

zhangfengcdt · 2024-07-26T15:42:53Z

CC: @jiayuasu @Kontinuation @wgtmac

parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/BoundingBox.java

...uet-column/src/main/java/org/apache/parquet/column/statistics/geometry/EnvelopeCovering.java

parquet-column/src/main/java/org/apache/parquet/column/statistics/BinaryStatistics.java

parquet-column/pom.xml

…e spherical edge is specified.

…apache-parquet-2417-geospatial

wgtmac

Thanks for the update! I have left some comments. I think we are reaching the finish line!

parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/GeometryUtils.java

parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/BoundingBox.java

parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/Covering.java

parquet-hadoop/pom.xml

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

parquet-hadoop/src/test/java/org/apache/parquet/statistics/TestGeometryTypeRoundTrip.java

wgtmac · 2024-08-17T07:36:23Z

parquet-hadoop/src/test/java/org/apache/parquet/statistics/TestGeometryTypeRoundTrip.java

+  }
+
+  @Test
+  public void testEPSG4326BasicReadWriteGeometryValue() throws Exception {


Thanks for adding these tests!

I think we are missing tests in following cases:

verify geometry type metadata is well preserved.

verify all kinds of geometry stats are preserved, including bbox, covering and geometry types.

verify geo stats in the column index have been generated.

I can do these later.

… module

wgtmac · 2025-04-30T15:31:40Z

Thanks! I will take a look after back from vacation.

...column/src/main/java/org/apache/parquet/column/statistics/geometry/GeospatialStatistics.java

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

- add valid flag to BoundingBox impl - check geostatistics validity when writing to parquet - add tests for mix of valid and invalid geometry update

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

Tested the following cases after the changes: - Merging valid stats with null stats results in invalid stats with null components - Merging null stats with valid stats also results in invalid stats with null components - Merging valid stats with partially null stats (null bounding box) results in the expected behavior where only the bounding box becomes null

jiayuasu · 2025-05-13T07:24:43Z

@wgtmac can you review again? Thank you!

parquet-column/src/main/java/org/apache/parquet/column/page/PageWriter.java

parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/BoundingBox.java

...column/src/main/java/org/apache/parquet/column/statistics/geometry/GeospatialStatistics.java

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

…ry, omit it from stats"

zhangfengcdt · 2025-05-14T14:53:04Z

@wgtmac Thank you for the very helpful review comments! I believe I’ve addressed them all and have also added and updated some tests. Could you please take another look and let me know if I missed anything or if further changes are needed? Thanks again!

wgtmac

Thanks for the quick update! Now it generally looks great.

parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/BoundingBox.java

...column/src/main/java/org/apache/parquet/column/statistics/geometry/GeospatialStatistics.java

parquet-column/src/test/java/org/apache/parquet/column/statistics/geometry/TestBoundingBox.java

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/BoundingBox.java

parquet-column/src/test/java/org/apache/parquet/column/statistics/geometry/TestBoundingBox.java

parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/GeospatialTypes.java

zhangfengcdt · 2025-05-16T04:56:26Z

Thanks for the quick update! Now it generally looks great.

Thanks for the review! The fixes should be all in now.

wgtmac

+1 Thanks!

...lumn/src/main/java/org/apache/parquet/column/statistics/geospatial/GeospatialStatistics.java

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

wgtmac · 2025-05-18T15:30:25Z

I just merged it. Thanks @zhangfengcdt for working on this and everyone for the review!

zhangfengcdt added 11 commits July 22, 2024 17:05

PARQUET-2471: Add support for geometry logical type

6a2e051

This PR is copied form this place: #1379

fix types

d354012

Refactor BoundingBox and GeometryTypes

969b696

revert naming changes

87ee8ea

revert TestDecimalUtils

d67e03b

revert more

ccf1c4a

refactor statistics

35342c2

modify EnvelopeCovering

bcfefb7

add more unit tests

cf615f6

update comments

c6ae733

add comment for envelope converging expand calculation

80a629e

zhangfengcdt marked this pull request as draft July 26, 2024 15:39

jiayuasu suggested changes Jul 26, 2024

View reviewed changes

parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/BoundingBox.java Outdated Show resolved Hide resolved

Fix the boundingbox initial values in constructor

e0ec9ef

wgtmac reviewed Aug 7, 2024

View reviewed changes

...uet-column/src/main/java/org/apache/parquet/column/statistics/geometry/EnvelopeCovering.java Outdated Show resolved Hide resolved

jiayuasu reviewed Aug 7, 2024

View reviewed changes

...uet-column/src/main/java/org/apache/parquet/column/statistics/geometry/EnvelopeCovering.java Outdated Show resolved Hide resolved

Update the poc implementation for the changes to the spec

7c728b1

jiayuasu reviewed Aug 7, 2024

View reviewed changes

parquet-column/src/main/java/org/apache/parquet/column/statistics/BinaryStatistics.java Outdated Show resolved Hide resolved

zhangfengcdt added 2 commits August 7, 2024 15:02

remove print

fa36d06

implement evelop covering for spherical coordinates

2a1c62a

wgtmac reviewed Aug 9, 2024

View reviewed changes

parquet-column/pom.xml Outdated Show resolved Hide resolved

zhangfengcdt added 3 commits August 12, 2024 08:52

throw a not-implemented exception for the covering statistics when th…

d0e7d3d

…e spherical edge is specified.

Merge branch 'master' of github.com:apache/parquet-java into feature-…

30d64be

…apache-parquet-2417-geospatial

remove unused comment codes

29a86b5

zhangfengcdt marked this pull request as ready for review August 12, 2024 15:57

wgtmac reviewed Aug 17, 2024

View reviewed changes

zhangfengcdt added 3 commits August 19, 2024 16:35

address some review comments

0c4b8b4

revert changes that are not desired

a56e9ad

refactor toString and remove test scope of jts-core in parquet-hadoop…

1ae3d99

… module

Kontinuation reviewed May 5, 2025

View reviewed changes

...column/src/main/java/org/apache/parquet/column/statistics/geometry/GeospatialStatistics.java Show resolved Hide resolved

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java Outdated Show resolved Hide resolved

address pr comments

ccd0715

- add valid flag to BoundingBox impl - check geostatistics validity when writing to parquet - add tests for mix of valid and invalid geometry update

Kontinuation reviewed May 6, 2025

View reviewed changes

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java Outdated Show resolved Hide resolved

zhangfengcdt added 3 commits May 12, 2025 06:49

address pr comment to only check the first coordinate for types update

ef8e343

change null or invalid only

e904e3e

wgtmac requested changes May 14, 2025

View reviewed changes

zhangfengcdt added 7 commits May 13, 2025 20:52

address review comments - part 1

882ef38

address review comments - part 2

e9d2426

address review comments

7955496

fix tests

74e94ab

update the test to reflect the changes of "Failed to parse WKB geomet…

9f3d31a

…ry, omit it from stats"

fix testInvalidGeometryPresented

136bbf4

fix spotless errors

945ecdf

wgtmac reviewed May 16, 2025

View reviewed changes

zhangfengcdt added 5 commits May 15, 2025 20:48

address review comments - part 1

f5dd1d0

add validity check before checkking any bbox values

dbc8798

split xy into x and y for valid and empty check

33d3b77

fix scala fmt issue

1957dca

rename geometry package to geospatial package

aec732e

wgtmac approved these changes May 16, 2025

View reviewed changes

...lumn/src/main/java/org/apache/parquet/column/statistics/geospatial/GeospatialStatistics.java Outdated Show resolved Hide resolved

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java Outdated Show resolved Hide resolved

clean up code

476dfff

jiayuasu approved these changes May 16, 2025

View reviewed changes

wgtmac changed the title ~~PARQUET-2417: Add support for geometry logical type~~ PARQUET-2417: Add statistics support to geometry logical type May 16, 2025

wgtmac merged commit 22a23d0 into apache:master May 18, 2025
7 checks passed

Conversation

zhangfengcdt commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jira

Tests

Commits

Documentation

Uh oh!

zhangfengcdt commented Jul 26, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac Aug 17, 2024

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Apr 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jiayuasu commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhangfengcdt commented May 14, 2025

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhangfengcdt commented May 16, 2025

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac commented May 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zhangfengcdt commented Jul 26, 2024 •

edited

Loading