address review comments

Author: zhangfengcdt

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

@@ -18,7 +18,6 @@
  */
 package org.apache.parquet.column.statistics.geometry;
 
-import org.apache.parquet.Preconditions;
 import org.apache.parquet.io.api.Binary;
 import org.apache.parquet.schema.LogicalTypeAnnotation;
 import org.apache.parquet.schema.PrimitiveType;
@@ -39,23 +38,6 @@ public class GeospatialStatistics {
   private BoundingBox boundingBox;
   private GeospatialTypes geospatialTypes;
 
-  /**
-   * Merge the statistics from another GeospatialStatistics object.
-   *
-   * @param other the other GeospatialStatistics object
-   */
-  public void mergeStatistics(GeospatialStatistics other) {
-    if (other == null) {
-      return;
-    }
-    if (this.boundingBox != null && other.boundingBox != null) {
-      this.boundingBox.merge(other.boundingBox);
-    }
-    if (this.geospatialTypes != null && other.geospatialTypes != null) {
-      this.geospatialTypes.merge(other.geospatialTypes);
-    }
-  }
-
   /**
    * Builder to create a GeospatialStatistics.
    */
@@ -170,28 +152,28 @@ public boolean isValid() {
     if (boundingBox == null || geospatialTypes == null) {
       return false;
     }
-    return boundingBox.isValid() && geospatialTypes.isValid();
+    return (boundingBox != null && boundingBox.isValid()) || (geospatialTypes != null && geospatialTypes.isValid());
   }
 
   public void merge(GeospatialStatistics other) {
-    Preconditions.checkArgument(other != null, "Cannot merge with null GeometryStatistics");
-
-    // If other bounding box is null or invalid, set this as null
-    if (boundingBox == null || other.boundingBox == null) {
-      boundingBox = null;
+    if (other == this) {
+      abort();
     }
 
     if (boundingBox != null) {
       boundingBox.merge(other.boundingBox);
     }
-
-    // If other geosptialTypes is null or invalid, set this as null
-    if (geospatialTypes == null || other.geospatialTypes == null) {
-      geospatialTypes = null;
+    if (geospatialTypes != null) {
+      geospatialTypes.merge(other.geospatialTypes);
     }
+  }
 
+  private void abort() {
+    if (boundingBox != null) {
+      boundingBox.abort();
+    }
     if (geospatialTypes != null) {
-      geospatialTypes.merge(other.geospatialTypes);
+      geospatialTypes.abort();
     }
   }
 

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

@@ -129,7 +129,7 @@ public void testCopyGeospatialStatistics() {
     builder.update(Binary.fromString("POINT (1 1)"));
     GeospatialStatistics statistics = builder.build();
     GeospatialStatistics copy = statistics.copy();
-    Assert.assertFalse(copy.isValid());
+    Assert.assertTrue(copy.isValid());
     Assert.assertNotNull(copy.getBoundingBox());
     Assert.assertNotNull(copy.getGeospatialTypes());
   }

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

@@ -821,12 +821,12 @@ private static BoundingBox toParquetBoundingBox(org.apache.parquet.column.statis
     formatBbox.setYmin(bbox.getYMin());
     formatBbox.setYmax(bbox.getYMax());
 
-    if (bbox.isZValid()) {
+    if (bbox.isZValid() && !bbox.isZEmpty()) {
       formatBbox.setZmin(bbox.getZMin());
       formatBbox.setZmax(bbox.getZMax());
     }
 
-    if (bbox.isMValid()) {
+    if (bbox.isMValid() && !bbox.isMEmpty()) {
       formatBbox.setMmin(bbox.getMMin());
       formatBbox.setMmax(bbox.getMMax());
     }
@@ -946,22 +946,29 @@ GeospatialStatistics toParquetGeospatialStatistics(
     }
 
     GeospatialStatistics formatStats = new GeospatialStatistics();
+    boolean isBoundingBoxSet = false;
+    boolean isGeometryTypes = false;
 
-    if (geospatialStatistics.getBoundingBox() != null) {
-      if (!geospatialStatistics.getBoundingBox().isValid()) {
-        return null;
-      }
-      if (geospatialStatistics.getBoundingBox().isXYEmpty()) {
-        return null;
-      }
+    if (geospatialStatistics.getBoundingBox() != null
+        && geospatialStatistics.getBoundingBox().isValid()
+        && !geospatialStatistics.getBoundingBox().isXYEmpty()) {
       formatStats.setBbox(toParquetBoundingBox(geospatialStatistics.getBoundingBox()));
+      isBoundingBoxSet = true;
     }
 
-    if (geospatialStatistics.getGeospatialTypes() != null) {
+    if (geospatialStatistics.getGeospatialTypes() != null
+        && geospatialStatistics.getGeospatialTypes().isValid()) {
       List<Integer> geometryTypes =
           new ArrayList<>(geospatialStatistics.getGeospatialTypes().getTypes());
-      Collections.sort(geometryTypes);
-      formatStats.setGeospatial_types(geometryTypes);
+      if (!geometryTypes.isEmpty()) {
+        Collections.sort(geometryTypes);
+        formatStats.setGeospatial_types(geometryTypes);
+        isGeometryTypes = true;
+      }
+    }
+
+    if (!isBoundingBoxSet && !isGeometryTypes) {
+      return null;
     }
 
     return formatStats;
@@ -1277,7 +1284,7 @@ LogicalTypeAnnotation getLogicalTypeAnnotation(ConvertedType type, SchemaElement
         return LogicalTypeAnnotation.bsonType();
       default:
         throw new RuntimeException(
-            "Can't convert converted type t o logical type, unknown converted type " + type);
+            "Can't convert converted type to logical type, unknown converted type " + type);
     }
   }
 
@@ -2512,7 +2519,6 @@ public static ColumnIndex toParquetColumnIndex(
     if (defLevelHistogram != null && !defLevelHistogram.isEmpty()) {
       parquetColumnIndex.setDefinition_level_histograms(defLevelHistogram);
     }
-
     return parquetColumnIndex;
   }
 

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ColumnChunkPageWriteStore.java

@@ -393,7 +393,7 @@ private void mergeColumnStatistics(
         sizeStatistics = null;
       }
 
-      totalGeospatialStatistics.mergeStatistics(geospatialStatistics);
+      totalGeospatialStatistics.merge(geospatialStatistics);
 
       if (totalStatistics != null && totalStatistics.isEmpty()) {
         return;