Re: [PR] PARQUET-2417: Add support for geometry logical type [parquet-java]

["wgtmac (via GitHub)" <gi...@apache.org>]

wgtmac commented on code in PR #2971:
URL: https://github.com/apache/parquet-java/pull/2971#discussion_r1748306996


##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/EnvelopeCovering.java:
##########
@@ -0,0 +1,225 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.locationtech.jts.geom.Coordinate;
+import org.locationtech.jts.geom.Envelope;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.GeometryFactory;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+import org.locationtech.jts.io.WKBWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class EnvelopeCovering extends Covering {
+
+  private static final Logger LOG = LoggerFactory.getLogger(EnvelopeCovering.class);
+
+  // The POC only supports EPSG:3857 and EPSG:4326 at the moment
+  private static final List<String> SUPPORTED_CRS = Arrays.asList("EPSG:3857", "EPSG:4326");
+
+  private static final ByteBuffer EMPTY = ByteBuffer.wrap(new byte[0]);
+  private final WKBReader reader = new WKBReader();
+  private final WKBWriter writer = new WKBWriter();
+  private final GeometryFactory factory = new GeometryFactory();
+  private LogicalTypeAnnotation.Edges edges;
+  private String crs;
+
+  public EnvelopeCovering(LogicalTypeAnnotation.Edges edges, String crs) {
+    super(EMPTY, DEFAULT_COVERING_KIND);
+    this.edges = edges;
+    this.crs = crs;
+    validateSupportedCrs(this.crs);
+    validateKind(this.kind);
+  }
+
+  public EnvelopeCovering() {
+    super(EMPTY, DEFAULT_COVERING_KIND);
+  }
+
+  private static void validateKind(String kind) {
+    Preconditions.checkArgument(kind.equalsIgnoreCase(DEFAULT_COVERING_KIND), "kind only accepts WKB");
+  }
+
+  private static void validateSupportedCrs(String crs) {
+    if (!SUPPORTED_CRS.contains(crs)) {
+      LOG.warn("Unsupported CRS: {}. Supported CRS are EPSG:3857 and EPSG:4326.", crs);

Review Comment:
   Why it does not throw?



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/EnvelopeCovering.java:
##########
@@ -0,0 +1,225 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.locationtech.jts.geom.Coordinate;
+import org.locationtech.jts.geom.Envelope;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.GeometryFactory;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+import org.locationtech.jts.io.WKBWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class EnvelopeCovering extends Covering {

Review Comment:
   Is it possible to make this class package private? We don't have to expose it as a public class. Users are sufficient to get the binary data and kind from its base class.



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/Covering.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import org.apache.parquet.Preconditions;
+import org.locationtech.jts.geom.Geometry;
+
+public abstract class Covering {
+  public static final String DEFAULT_COVERING_KIND = "WKB";

Review Comment:
   This is a public field but only used in `EnvelopeCovering`. I'd suggest to remove it from here.



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/GeometryStatistics.java:
##########
@@ -0,0 +1,200 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.*;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.io.api.Binary;
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+
+public class GeometryStatistics {
+
+  // Metadata that may impact the statistics calculation
+  private final LogicalTypeAnnotation.Edges edges;
+  private final String crs;
+  private final ByteBuffer metadata;
+
+  private final BoundingBox boundingBox;
+  private final Map<String, Covering> coverings;
+  private final GeometryTypes geometryTypes;
+  private final WKBReader reader = new WKBReader();
+
+  public GeometryStatistics(
+      LogicalTypeAnnotation.Edges edges,
+      String crs,
+      ByteBuffer metadata,
+      BoundingBox boundingBox,
+      List<Covering> coverings,
+      GeometryTypes geometryTypes) {
+    this.edges = edges;
+    this.crs = crs;
+    this.metadata = metadata;
+    this.boundingBox = supportsBoundingBox() ? boundingBox : null;
+    this.coverings = supportsCovering() ? new HashMap<>() : null;
+    this.geometryTypes = geometryTypes;
+
+    if (this.coverings != null && coverings != null) {
+      for (Covering covering : coverings) {
+        // Assuming each Covering has a unique identifier (kind) or property that can be used as a key
+        String key = covering.getKind(); // Assume kind is unique
+        this.coverings.put(key, covering);

Review Comment:
   Check if `this.coverings.size() == coverings.size()` to enforce uniqueness?



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/EnvelopeCovering.java:
##########
@@ -0,0 +1,225 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.locationtech.jts.geom.Coordinate;
+import org.locationtech.jts.geom.Envelope;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.GeometryFactory;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+import org.locationtech.jts.io.WKBWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class EnvelopeCovering extends Covering {
+
+  private static final Logger LOG = LoggerFactory.getLogger(EnvelopeCovering.class);
+
+  // The POC only supports EPSG:3857 and EPSG:4326 at the moment
+  private static final List<String> SUPPORTED_CRS = Arrays.asList("EPSG:3857", "EPSG:4326");
+
+  private static final ByteBuffer EMPTY = ByteBuffer.wrap(new byte[0]);
+  private final WKBReader reader = new WKBReader();
+  private final WKBWriter writer = new WKBWriter();
+  private final GeometryFactory factory = new GeometryFactory();
+  private LogicalTypeAnnotation.Edges edges;
+  private String crs;
+
+  public EnvelopeCovering(LogicalTypeAnnotation.Edges edges, String crs) {
+    super(EMPTY, DEFAULT_COVERING_KIND);

Review Comment:
   See my previous comment. We can add a `static final String WKB_COVERING_KIND = "WKB";` in this class instead of super class.



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/GeometryStatistics.java:
##########
@@ -0,0 +1,200 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.*;

Review Comment:
   Please avoid wildcard import.



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/EnvelopeCovering.java:
##########
@@ -0,0 +1,225 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.locationtech.jts.geom.Coordinate;
+import org.locationtech.jts.geom.Envelope;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.GeometryFactory;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+import org.locationtech.jts.io.WKBWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class EnvelopeCovering extends Covering {
+
+  private static final Logger LOG = LoggerFactory.getLogger(EnvelopeCovering.class);
+
+  // The POC only supports EPSG:3857 and EPSG:4326 at the moment
+  private static final List<String> SUPPORTED_CRS = Arrays.asList("EPSG:3857", "EPSG:4326");
+
+  private static final ByteBuffer EMPTY = ByteBuffer.wrap(new byte[0]);
+  private final WKBReader reader = new WKBReader();
+  private final WKBWriter writer = new WKBWriter();
+  private final GeometryFactory factory = new GeometryFactory();
+  private LogicalTypeAnnotation.Edges edges;
+  private String crs;
+
+  public EnvelopeCovering(LogicalTypeAnnotation.Edges edges, String crs) {
+    super(EMPTY, DEFAULT_COVERING_KIND);
+    this.edges = edges;
+    this.crs = crs;
+    validateSupportedCrs(this.crs);
+    validateKind(this.kind);
+  }
+
+  public EnvelopeCovering() {
+    super(EMPTY, DEFAULT_COVERING_KIND);
+  }
+
+  private static void validateKind(String kind) {
+    Preconditions.checkArgument(kind.equalsIgnoreCase(DEFAULT_COVERING_KIND), "kind only accepts WKB");
+  }
+
+  private static void validateSupportedCrs(String crs) {
+    if (!SUPPORTED_CRS.contains(crs)) {
+      LOG.warn("Unsupported CRS: {}. Supported CRS are EPSG:3857 and EPSG:4326.", crs);
+    }
+  }
+
+  @Override
+  public String getKind() {
+    return kind + "|" + crs + "|" + edges;
+  }
+
+  @Override
+  public void setKind(String kind) {
+    if (kind == null || kind.isEmpty()) {

Review Comment:
   This looks weird to me since it includes more information than `kind`. From the specs, `crs` and `edges` should be deduced from the geometry logical type metadata.



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/GeometryStatistics.java:
##########
@@ -0,0 +1,200 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.*;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.io.api.Binary;
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+
+public class GeometryStatistics {
+
+  // Metadata that may impact the statistics calculation
+  private final LogicalTypeAnnotation.Edges edges;
+  private final String crs;
+  private final ByteBuffer metadata;
+
+  private final BoundingBox boundingBox;
+  private final Map<String, Covering> coverings;
+  private final GeometryTypes geometryTypes;
+  private final WKBReader reader = new WKBReader();
+
+  public GeometryStatistics(
+      LogicalTypeAnnotation.Edges edges,
+      String crs,
+      ByteBuffer metadata,
+      BoundingBox boundingBox,
+      List<Covering> coverings,
+      GeometryTypes geometryTypes) {
+    this.edges = edges;
+    this.crs = crs;
+    this.metadata = metadata;
+    this.boundingBox = supportsBoundingBox() ? boundingBox : null;
+    this.coverings = supportsCovering() ? new HashMap<>() : null;
+    this.geometryTypes = geometryTypes;
+
+    if (this.coverings != null && coverings != null) {
+      for (Covering covering : coverings) {
+        // Assuming each Covering has a unique identifier (kind) or property that can be used as a key
+        String key = covering.getKind(); // Assume kind is unique
+        this.coverings.put(key, covering);
+      }
+    }
+  }
+
+  public GeometryStatistics(LogicalTypeAnnotation.Edges edges, String crs, ByteBuffer metadata) {
+    this(
+        edges,
+        crs,
+        metadata,
+        new BoundingBox(),
+        Arrays.asList(new EnvelopeCovering(edges, crs)),
+        new GeometryTypes());
+  }
+
+  public BoundingBox getBoundingBox() {
+    return boundingBox;
+  }
+
+  public Map<String, Covering> getCoverings() {
+    return coverings;
+  }
+
+  public GeometryTypes getGeometryTypes() {
+    return geometryTypes;
+  }
+
+  public void update(Binary value) {
+    if (value == null) {
+      return;
+    }
+    try {
+      Geometry geom = reader.read(value.getBytes());
+      update(geom);
+    } catch (ParseException e) {
+      abort();
+    }
+  }
+
+  private void update(Geometry geom) {
+    if (supportsBoundingBox()) {
+      boundingBox.update(geom);
+    }
+    if (supportsCovering()) {
+      coverings.values().stream().forEach(c -> c.update(geom));
+    }
+    geometryTypes.update(geom);
+  }
+
+  /**
+   * A bounding box is a rectangular region defined by two points, the lower left
+   * and upper right corners. It is used to represent the minimum and maximum
+   * coordinates of a geometry. Only planar geometries can have a bounding box.
+   */
+  private boolean supportsBoundingBox() {
+    // Only planar geometries can have a bounding box
+    // based on the current specification
+    return edges == LogicalTypeAnnotation.Edges.PLANAR;
+  }
+
+  /**
+   * A custom WKB-encoded polygon or multi-polygon to represent a covering of
+   * geometries. For example, it may be a bounding box, or an envelope of geometries
+   * when a bounding box cannot be built (e.g. a geometry has spherical edges, or if
+   * an edge of geographic coordinates crosses the antimeridian). In addition, it can
+   * also be used to provide vendor-agnostic coverings like S2 or H3 grids.
+   */
+  private boolean supportsCovering() {
+    // This version (POC) assumes only build coverings for planar edges

Review Comment:
   Please remove any `POC` from the comment. Otherwise, we might forget to remove them later.



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/EnvelopeCovering.java:
##########
@@ -0,0 +1,225 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.locationtech.jts.geom.Coordinate;
+import org.locationtech.jts.geom.Envelope;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.GeometryFactory;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+import org.locationtech.jts.io.WKBWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class EnvelopeCovering extends Covering {
+
+  private static final Logger LOG = LoggerFactory.getLogger(EnvelopeCovering.class);
+
+  // The POC only supports EPSG:3857 and EPSG:4326 at the moment
+  private static final List<String> SUPPORTED_CRS = Arrays.asList("EPSG:3857", "EPSG:4326");
+
+  private static final ByteBuffer EMPTY = ByteBuffer.wrap(new byte[0]);
+  private final WKBReader reader = new WKBReader();
+  private final WKBWriter writer = new WKBWriter();
+  private final GeometryFactory factory = new GeometryFactory();
+  private LogicalTypeAnnotation.Edges edges;
+  private String crs;
+
+  public EnvelopeCovering(LogicalTypeAnnotation.Edges edges, String crs) {
+    super(EMPTY, DEFAULT_COVERING_KIND);
+    this.edges = edges;
+    this.crs = crs;
+    validateSupportedCrs(this.crs);
+    validateKind(this.kind);
+  }
+
+  public EnvelopeCovering() {
+    super(EMPTY, DEFAULT_COVERING_KIND);
+  }
+
+  private static void validateKind(String kind) {
+    Preconditions.checkArgument(kind.equalsIgnoreCase(DEFAULT_COVERING_KIND), "kind only accepts WKB");
+  }
+
+  private static void validateSupportedCrs(String crs) {
+    if (!SUPPORTED_CRS.contains(crs)) {
+      LOG.warn("Unsupported CRS: {}. Supported CRS are EPSG:3857 and EPSG:4326.", crs);
+    }
+  }
+
+  @Override
+  public String getKind() {
+    return kind + "|" + crs + "|" + edges;
+  }
+
+  @Override
+  public void setKind(String kind) {
+    if (kind == null || kind.isEmpty()) {
+      throw new IllegalArgumentException("Kind cannot be null or empty");
+    }
+
+    // Split the input string by the "|" delimiter
+    String[] parts = kind.split("\\|");
+
+    // Ensure we have exactly 3 parts: kind, crs, and edges
+    if (parts.length != 3) {
+      throw new IllegalArgumentException("Invalid kind format. Expected format: 'kind|crs|edges'");
+    }
+
+    // Assign the values to the respective fields
+    this.kind = parts[0];
+    this.crs = parts[1];
+
+    try {
+      this.edges = LogicalTypeAnnotation.Edges.valueOf(parts[2]);
+    } catch (IllegalArgumentException e) {
+      throw new IllegalArgumentException("Invalid edges value: " + parts[2], e);
+    }
+  }
+
+  @Override
+  void update(Geometry geom) {
+    if (geom == null) {
+      return;
+    }
+    try {
+      if (value != EMPTY) {
+        Geometry existingGeometry = reader.read(value.array());
+        Envelope existingEnvelope = createEnvelopeFromPolygon(existingGeometry);
+        Envelope newEnvelope = geom.getEnvelopeInternal();
+
+        Envelope combinedEnvelope = edges == LogicalTypeAnnotation.Edges.SPHERICAL
+            ? extendEnvelopeSpherical(existingEnvelope, newEnvelope)
+            : extendEnvelopePlanar(existingEnvelope, newEnvelope);
+
+        Geometry envelopePolygon = createPolygonFromEnvelope(combinedEnvelope);
+
+        value = ByteBuffer.wrap(writer.write(envelopePolygon));
+      } else {
+        Geometry envelopePolygon = createPolygonFromEnvelope(geom.getEnvelopeInternal());
+        value = ByteBuffer.wrap(writer.write(envelopePolygon));
+      }
+    } catch (ParseException e) {
+      value = null;

Review Comment:
   Add an error log for what has happened?



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/EnvelopeCovering.java:
##########
@@ -0,0 +1,225 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.locationtech.jts.geom.Coordinate;
+import org.locationtech.jts.geom.Envelope;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.GeometryFactory;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+import org.locationtech.jts.io.WKBWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class EnvelopeCovering extends Covering {
+
+  private static final Logger LOG = LoggerFactory.getLogger(EnvelopeCovering.class);
+
+  // The POC only supports EPSG:3857 and EPSG:4326 at the moment
+  private static final List<String> SUPPORTED_CRS = Arrays.asList("EPSG:3857", "EPSG:4326");
+
+  private static final ByteBuffer EMPTY = ByteBuffer.wrap(new byte[0]);
+  private final WKBReader reader = new WKBReader();
+  private final WKBWriter writer = new WKBWriter();
+  private final GeometryFactory factory = new GeometryFactory();
+  private LogicalTypeAnnotation.Edges edges;
+  private String crs;
+
+  public EnvelopeCovering(LogicalTypeAnnotation.Edges edges, String crs) {
+    super(EMPTY, DEFAULT_COVERING_KIND);
+    this.edges = edges;
+    this.crs = crs;
+    validateSupportedCrs(this.crs);
+    validateKind(this.kind);
+  }
+
+  public EnvelopeCovering() {
+    super(EMPTY, DEFAULT_COVERING_KIND);
+  }
+
+  private static void validateKind(String kind) {
+    Preconditions.checkArgument(kind.equalsIgnoreCase(DEFAULT_COVERING_KIND), "kind only accepts WKB");
+  }
+
+  private static void validateSupportedCrs(String crs) {
+    if (!SUPPORTED_CRS.contains(crs)) {
+      LOG.warn("Unsupported CRS: {}. Supported CRS are EPSG:3857 and EPSG:4326.", crs);
+    }
+  }
+
+  @Override
+  public String getKind() {
+    return kind + "|" + crs + "|" + edges;

Review Comment:
   See my comment below this one. This seems to break the contract from base class.



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/GeometryStatistics.java:
##########
@@ -0,0 +1,200 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.*;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.io.api.Binary;
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+
+public class GeometryStatistics {
+
+  // Metadata that may impact the statistics calculation
+  private final LogicalTypeAnnotation.Edges edges;
+  private final String crs;
+  private final ByteBuffer metadata;
+
+  private final BoundingBox boundingBox;
+  private final Map<String, Covering> coverings;
+  private final GeometryTypes geometryTypes;
+  private final WKBReader reader = new WKBReader();
+
+  public GeometryStatistics(
+      LogicalTypeAnnotation.Edges edges,
+      String crs,
+      ByteBuffer metadata,
+      BoundingBox boundingBox,
+      List<Covering> coverings,
+      GeometryTypes geometryTypes) {
+    this.edges = edges;
+    this.crs = crs;
+    this.metadata = metadata;
+    this.boundingBox = supportsBoundingBox() ? boundingBox : null;
+    this.coverings = supportsCovering() ? new HashMap<>() : null;

Review Comment:
   Could we use an immutable dummy object instead of null for `this.boundingBox` and `this.coverings` here? I saw methods like `abort()` and `reset()` below do not check null before calling, which may result in NPE. An alternative is to check `supportsXXX()` in every method.



##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/geometry/EnvelopeCovering.java:
##########
@@ -0,0 +1,225 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.column.statistics.geometry;
+
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.locationtech.jts.geom.Coordinate;
+import org.locationtech.jts.geom.Envelope;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.GeometryFactory;
+import org.locationtech.jts.io.ParseException;
+import org.locationtech.jts.io.WKBReader;
+import org.locationtech.jts.io.WKBWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class EnvelopeCovering extends Covering {
+
+  private static final Logger LOG = LoggerFactory.getLogger(EnvelopeCovering.class);
+
+  // The POC only supports EPSG:3857 and EPSG:4326 at the moment
+  private static final List<String> SUPPORTED_CRS = Arrays.asList("EPSG:3857", "EPSG:4326");
+
+  private static final ByteBuffer EMPTY = ByteBuffer.wrap(new byte[0]);
+  private final WKBReader reader = new WKBReader();
+  private final WKBWriter writer = new WKBWriter();
+  private final GeometryFactory factory = new GeometryFactory();
+  private LogicalTypeAnnotation.Edges edges;
+  private String crs;
+
+  public EnvelopeCovering(LogicalTypeAnnotation.Edges edges, String crs) {
+    super(EMPTY, DEFAULT_COVERING_KIND);
+    this.edges = edges;
+    this.crs = crs;
+    validateSupportedCrs(this.crs);
+    validateKind(this.kind);
+  }
+
+  public EnvelopeCovering() {
+    super(EMPTY, DEFAULT_COVERING_KIND);
+  }
+
+  private static void validateKind(String kind) {
+    Preconditions.checkArgument(kind.equalsIgnoreCase(DEFAULT_COVERING_KIND), "kind only accepts WKB");
+  }
+
+  private static void validateSupportedCrs(String crs) {
+    if (!SUPPORTED_CRS.contains(crs)) {
+      LOG.warn("Unsupported CRS: {}. Supported CRS are EPSG:3857 and EPSG:4326.", crs);
+    }
+  }
+
+  @Override
+  public String getKind() {
+    return kind + "|" + crs + "|" + edges;
+  }
+
+  @Override
+  public void setKind(String kind) {
+    if (kind == null || kind.isEmpty()) {
+      throw new IllegalArgumentException("Kind cannot be null or empty");
+    }
+
+    // Split the input string by the "|" delimiter
+    String[] parts = kind.split("\\|");
+
+    // Ensure we have exactly 3 parts: kind, crs, and edges
+    if (parts.length != 3) {
+      throw new IllegalArgumentException("Invalid kind format. Expected format: 'kind|crs|edges'");
+    }
+
+    // Assign the values to the respective fields
+    this.kind = parts[0];
+    this.crs = parts[1];
+
+    try {
+      this.edges = LogicalTypeAnnotation.Edges.valueOf(parts[2]);
+    } catch (IllegalArgumentException e) {
+      throw new IllegalArgumentException("Invalid edges value: " + parts[2], e);
+    }
+  }
+
+  @Override
+  void update(Geometry geom) {
+    if (geom == null) {
+      return;
+    }
+    try {
+      if (value != EMPTY) {
+        Geometry existingGeometry = reader.read(value.array());
+        Envelope existingEnvelope = createEnvelopeFromPolygon(existingGeometry);
+        Envelope newEnvelope = geom.getEnvelopeInternal();
+
+        Envelope combinedEnvelope = edges == LogicalTypeAnnotation.Edges.SPHERICAL
+            ? extendEnvelopeSpherical(existingEnvelope, newEnvelope)
+            : extendEnvelopePlanar(existingEnvelope, newEnvelope);
+
+        Geometry envelopePolygon = createPolygonFromEnvelope(combinedEnvelope);
+
+        value = ByteBuffer.wrap(writer.write(envelopePolygon));
+      } else {
+        Geometry envelopePolygon = createPolygonFromEnvelope(geom.getEnvelopeInternal());
+        value = ByteBuffer.wrap(writer.write(envelopePolygon));
+      }
+    } catch (ParseException e) {
+      value = null;

Review Comment:
   BYW, should we catch all exceptions here? We don't want to fail the writing if anything wrong happens here, e.g. `extendEnvelopeSpherical` might throw for spherical features.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org