PARQUET-2471: Add support for geometry logical type #1379

wgtmac · 2024-06-19T16:01:13Z

This is the PoC of apache/parquet-format#240

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-XXX
- In case you are adding a dependency, check if the license complies with
  the ASF 3rd Party License Policy.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

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"

Style

My contribution adheres to the code style guidelines and Spotless passes.
- To apply the necessary changes, run mvn spotless:apply -Pvector-plugins

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

wgtmac · 2024-06-20T06:46:44Z

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

+      Assert.assertNotNull(geometryStatistics);
+      System.out.println("GeometryStatistics");
+      System.out.println(geometryStatistics);
+      System.out.println();


I have just finished the PoC to read/write geometry values in the form of WKB and verify the GeometryStatistics have been collected as expected. This is the output of this minimal test.

Footer ParquetMetaData{FileMetaData{schema: message msg { required binary col_geom (GEOMETRY(WKB,PLANAR,EPSG:4326)); } , metadata: {writer.model.name=example}}, blocks: [BlockMetaData{2, 73, rowIndexOffset = 0 [ColumnMetaData{UNCOMPRESSED [col_geom] required binary col_geom (GEOMETRY(WKB,PLANAR,EPSG:4326)) [BIT_PACKED, PLAIN], 4}]}]} Statistics min: POINT (1 1), max: POINT (2 2), num_nulls: 0 GeometryStatistics GeometryStatistics{boundingBox=BoundingBox{xMin=1.0, xMax=2.0, yMin=1.0, yMax=2.0, zMin=NaN, zMax=NaN, mMin=NaN, mMax=NaN}, covering=Covering{geometry=POLYGON ((1 1, 1 2, 2 2, 2 1, 1 1)), edges=PLANAR}, geometryTypes=GeometryTypes{types=[1]}}

@szehon-ho @jiayuasu @paleolimbot @emkornfield

Please let me know what do you think. Thanks!

Could you help take an initial review to see if this looks promising? @gszadovszky @julienledem

@wgtmac, I am not really familiar with this new geometry logical type. So, my comments are more general.

I can see a TODO for the comparator but I cannot see any specification in the format PR about the total ordering. (BTW, I cannot see a proper spec in LogicalTypes.md. It is for the other PR, though.)

Could you please double-check the licensing of org.locationtech.jts whether we are able to distribute parquet-java as is? I am not expert of this, though.

I think, it is not a good idea to include external objects (org.locationtech.jts.geom.Geometry) into our public API. In our public API we should stick to the primitive type (Binary).

Otherwise, it looks good to me.

Thanks @gszadovszky for a quick review! Please see my inline comments below.

I can see a TODO for the comparator but I cannot see any specification in the format PR about the total ordering.

I thought we'd better make the ColumnOrder of geometry logical type as undefined and do not populate min/max values. Here is the related line in the format PR: https://github.com/apache/parquet-format/pull/240/files#diff-834c5a8d91719350b20995ad99d1cb6d8d68332b9ac35694f40e375bdb2d3e7cR1083

Could you please double-check the licensing of org.locationtech.jts whether we are able to distribute parquet-java as is?

It has dual licenses:

Eclipse Public License 2.0

Eclipse Distribution License 1.0 (a BSD Style License).

I have also checked Apache Sedona which includes org.locationtech.jts/jts-core in its dependency: https://github.com/apache/sedona/blob/119682321f5301e6ef776315a8f2cd53a8ff63b0/pom.xml#L168-L188. So I assume we are safe here.

@jiayuasu may provide more context as the domain expert.

I think, it is not a good idea to include external objects (org.locationtech.jts.geom.Geometry) into our public API. In our public API we should stick to the primitive type (Binary).

I agree that it is not a good idea to expose org.locationtech.jts.geom.Geometry as a public API. Let me switch it to internal method for now. If downstream projects like Apache Iceberg (which has customized column writer implementation) could benefit from directly passing a org.locationtech.jts.geom.Geometry to build stats without unnecessary conversion, we can revisit this later.

Thanks for the answers, @wgtmac!
The TODO comment about the comparator can be removed, then. With the moving of the Geometry references into non-public scopes I'm OK with this.

Please let me know what do you think

This looks good to me! (I left some geometry-specific notes on the implementation)

I have also checked Apache Sedona which includes org.locationtech.jts/jts-core in its dependency

I believe Calcite also uses JTS for its spatial component and it may be worth checking there to see to what extent those types are used in various APIs and/or if it's ever caused any issues.

I am pretty confident that JTS is fully compatible with ASF license: https://www.apache.org/legal/resolved.html

This is also why Sedona added it years ago

wgtmac · 2024-06-20T06:52:17Z

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

+
+public class GeometryStatistics {
+
+  private final BoundingBox boundingBox;


GeometryStatistics is something that I can hardly find a way to fit into an extension type. Please let me know if you have any better idea. @pitrou

paleolimbot

I took a look through for geometry things...this is very cool!

paleolimbot · 2024-06-21T12:31:28Z

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

+    if (typeId == UNKNOWN_TYPE_ID) {
+      return UNKNOWN_TYPE_ID;
+    }
+    Coordinate coordinate = geometry.getCoordinate();


I'm surprised that a Geometry doesn't have a getDimensions or similar

paleolimbot · 2024-06-21T12:34:58Z

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

+    for (Coordinate coordinate : coordinates) {
+      update(coordinate.getX(), coordinate.getY(), coordinate.getZ(), coordinate.getM());
+    }


I am surprised that JTS doesn't have an optimized method for this (it almost certainly has at least an internal one for building indexes internally)

paleolimbot · 2024-06-21T12:51:22Z

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

+    formatStats.setGeometry_types(
+        new ArrayList<Integer>(stats.getGeometryTypes().getTypes()));


It may be worth sorting this here (if the hash set used in the geometry types has non-deterministic ordering)

Make sense. Fixed.

paleolimbot · 2024-06-21T13:18:43Z

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

+    try {
+      if (geometry != EMPTY) {
+        Geometry envelope = reader.read(geometry.array());
+        geometry = ByteBuffer.wrap(writer.write(envelope.union(geom).getEnvelope()));


I am a little unclear as to the utility of the EnvelopeCovering...it seems like the optimal strategy would be to always merge BoundingBoxes and build a geometry at the end. A union in a loop is not usually efficient and should perhaps be more difficult to accidentally do.

I agree. Perhaps I can call Envelope.expandToInclude(Envelope) to maintain a running envelope.

https://locationtech.github.io/jts/javadoc/org/locationtech/jts/geom/Envelope.html#expandToInclude-org.locationtech.jts.geom.Envelope-

wgtmac · 2024-06-22T15:12:45Z

Thanks @paleolimbot for the quick review! I've addressed your feedback and also added page index support for geometry stats. BTW, it is a convention that we need PoC on another Parquet implementation to move forward the proposal. I plan to add similar things to parquet-cpp. Do you happen to know any JTS-parity on the C++ side? Or do you have a plan to implement the PoC? I guess there should be a lot of existing code on the GeoParquet side.

jiayuasu · 2024-06-23T05:30:48Z

@wgtmac The JTS-parity on C++ side is GEOS, which is a C++ port of JTS: https://github.com/libgeos/geos

Note that: GEOS is LGPL 2.1, which is NOT compatible with ASF

jiayuasu · 2024-06-23T05:35:14Z

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

+    return mMax;
+  }
+
+  void update(Geometry geom) {


This is not the correct way to update Geometry in JTS. You need to use CoordinateSequenceFilter. Sedona has many examples here: https://github.com/apache/sedona/blob/0a1db3d35fd7b5721301aa3ce5d39aa8cd828661/common/src/main/java/org/apache/sedona/common/Functions.java#L294

jiayuasu · 2024-06-23T05:38:00Z

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

+public class BoundingBox {
+
+  private double xMin = Double.MAX_VALUE;
+  private double xMax = Double.MIN_VALUE;


For the init value of xMax, yMax, ..., please use -Double.MAX_VALUE because Double.MIN_VALUE is not guaranteed to be negative. See https://stackoverflow.com/questions/3884793/why-is-double-min-value-in-not-negative

Double.POSITIVE_INFINITY and Double.NEGATIVE_INFINITY are also valid choices for initial min/max values.

For extracting the bounds for each ordinate, org.locationtech.jts.io.twkb.BoundsExtractor in the JTS code base is also a good example.

jiayuasu · 2024-06-23T05:53:37Z

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

+          new Coordinate(envelope.getMaxX(), envelope.getMinY()),
+          new Coordinate(envelope.getMinX(), envelope.getMinY())
+        });
+        geometry = ByteBuffer.wrap(writer.write(polygon));


Pay attention to a trap here: JTS WKB writer has a bug that does not write M value. There is a PR on JTS to fix this but it hasn't been merged: locationtech/jts#734.

I think we also need to be a bit more explicit here about which WKB standard we are supporting.

Standard WKB format does not allow Z and M dimension. ISO WKB allow Z and M dimension. JTS follows ISO WKB but its writer has the aforementioned bug. See this article from GEOS: https://libgeos.org/specifications/wkb/

Thanks for the advise! BTW, it seems that JTS envelope has only X and Y axises. I'm not sure if this is another issue here.

jiayuasu · 2024-06-23T05:58:13Z

Core sedona contributors @Kontinuation and @zhangfengcdt , can you also review this?

@wgtmac Thanks for putting together this PR. I guess you might need some help from the Sedona community regarding the Java geometry implementation together with JTS. We can work together to implement this POC.

wgtmac · 2024-06-23T14:43:19Z

Thanks @jiayuasu for your valuable feedback! Yes, it would be great if we can get direct help from geospatial experts!

paleolimbot · 2024-06-24T01:49:24Z

Do you happen to know any JTS-parity on the C++ side?

As Jia mentioned, it's GEOS, but this is not something the C++ implementation can/should take on as a dependency.

I guess there should be a lot of existing code on the GeoParquet side.

GDAL has quite a lot to draw from ( https://github.com/OSGeo/gdal/tree/master/ogr/ogrsf_frmts/parquet ), although I think pretty much all we need to do is walk WKB for bounding boxes and geometry type ( https://github.com/OSGeo/gdal/blob/12dab86ca2d8b1a65c4c085e137c62294682ac1d/ogr/ogr_wkb.cpp#L387-L584 ).

Or do you have a plan to implement the PoC?

I am happy to give it a shot (I'll make a start this week).

Kontinuation · 2024-06-24T03:49:27Z

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

+public class BoundingBox {
+
+  private double xMin = Double.MAX_VALUE;
+  private double xMax = Double.MIN_VALUE;


Double.POSITIVE_INFINITY and Double.NEGATIVE_INFINITY are also valid choices for initial min/max values.

For extracting the bounds for each ordinate, org.locationtech.jts.io.twkb.BoundsExtractor in the JTS code base is also a good example.

Kontinuation · 2024-06-24T03:55:38Z

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

+        Geometry polygon = factory.createPolygon(new Coordinate[] {
+          new Coordinate(envelope.getMinX(), envelope.getMinY()),
+          new Coordinate(envelope.getMinX(), envelope.getMaxY()),
+          new Coordinate(envelope.getMaxX(), envelope.getMaxY()),
+          new Coordinate(envelope.getMaxX(), envelope.getMinY()),
+          new Coordinate(envelope.getMinX(), envelope.getMinY())
+        });


If I understand it correctly, EnvelopeCovering repliates BoundingBox, so can we reuse BoundingBox here to handle all ordinates correctly?

If we still use geom.getEnvelopeInternal, we can replace this with a shorter version factory.toGeometry(envelope).

EnvelopeCovering is just a possible implementation of the Covering. The idea behind putting Covering and BoundingBox at the same time originates from the suggestion of apache/parquet-format#240 (comment) and apache/parquet-format#240 (comment)

Kontinuation · 2024-06-24T04:41:37Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

+      if (crs != null && !crs.isEmpty()) {
+        sb.append(",");
+        sb.append(crs);
+      }
+      if (metadata != null) {
+        sb.append(",");
+        sb.append(metadata);
+      }


There's a high probability that crs itself contains comma, so this may introduce ambiguity to the generated type parameters.

metadata will be appended as something like java.nio.HeapByteBuffer[pos=_ lim=_ cap=_] to the type parameter. Does this have any bad implications?

This is definitely something that I need to improve :)

PARQUET-2471: Add support for geometry logical type

4f24986

wgtmac force-pushed the geospatial branch from 4f21474 to 4f24986 Compare June 20, 2024 06:41

wgtmac commented Jun 20, 2024

View reviewed changes

make method with Geometry param private

36f93dc

paleolimbot reviewed Jun 21, 2024

View reviewed changes

wgtmac added 2 commits June 22, 2024 22:52

implement geom stats of page index

2f2e154

sort geometry types before writing

cac953a

jiayuasu suggested changes Jun 23, 2024

View reviewed changes

Kontinuation reviewed Jun 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PARQUET-2471: Add support for geometry logical type #1379

PARQUET-2471: Add support for geometry logical type #1379

wgtmac commented Jun 19, 2024 •

edited

Loading

wgtmac Jun 20, 2024

wgtmac Jun 20, 2024

gszadovszky Jun 20, 2024

wgtmac Jun 21, 2024

gszadovszky Jun 21, 2024

paleolimbot Jun 21, 2024

jiayuasu Jun 23, 2024

wgtmac Jun 20, 2024 •

edited

Loading

paleolimbot left a comment

paleolimbot Jun 21, 2024

paleolimbot Jun 21, 2024

paleolimbot Jun 21, 2024

wgtmac Jun 22, 2024

paleolimbot Jun 21, 2024

wgtmac Jun 21, 2024

wgtmac commented Jun 22, 2024

jiayuasu commented Jun 23, 2024

jiayuasu Jun 23, 2024

jiayuasu Jun 23, 2024

Kontinuation Jun 24, 2024

jiayuasu Jun 23, 2024

wgtmac Jun 23, 2024

jiayuasu commented Jun 23, 2024

wgtmac commented Jun 23, 2024

paleolimbot commented Jun 24, 2024

Kontinuation Jun 24, 2024

Kontinuation Jun 24, 2024

wgtmac Jun 25, 2024

Kontinuation Jun 24, 2024

wgtmac Jun 25, 2024


		public class GeometryStatistics {

		private final BoundingBox boundingBox;

		formatStats.setGeometry_types(
		new ArrayList<Integer>(stats.getGeometryTypes().getTypes()));

PARQUET-2471: Add support for geometry logical type #1379

Are you sure you want to change the base?

PARQUET-2471: Add support for geometry logical type #1379

Conversation

wgtmac commented Jun 19, 2024 • edited Loading

Jira

Tests

Commits

Style

Documentation

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 Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

paleolimbot 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

wgtmac commented Jun 22, 2024

jiayuasu commented Jun 23, 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

jiayuasu commented Jun 23, 2024

wgtmac commented Jun 23, 2024

paleolimbot commented Jun 24, 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

wgtmac commented Jun 19, 2024 •

edited

Loading

wgtmac Jun 20, 2024 •

edited

Loading