Fix review issues

apache · Mar 7, 2025 · fc193c6 · fc193c6
1 parent c3b57f7
commit fc193c6
Show file tree

Hide file tree

Showing 8 changed files with 35 additions and 11 deletions.
diff --git a/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java b/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java
@@ -19,6 +19,7 @@
 package org.apache.iceberg.types;
 
 import java.util.Locale;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 
 /** The algorithm for interpolating edges. */
 public enum EdgeInterpolationAlgorithm {
@@ -54,11 +55,12 @@ public String value() {
   }
 
   public static EdgeInterpolationAlgorithm fromName(String algorithmName) {
+    Preconditions.checkNotNull(algorithmName, "Edge interpolation algorithm cannot be null");
     try {
       return EdgeInterpolationAlgorithm.valueOf(algorithmName.toUpperCase(Locale.ENGLISH));
     } catch (IllegalArgumentException e) {
       throw new IllegalArgumentException(
-          String.format("Invalid edge interpolation algorithm name: %s", algorithmName), e);
+          String.format("Invalid edge interpolation algorithm: %s", algorithmName), e);
     }
   }
 }
diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java
@@ -58,6 +58,8 @@ private Types() {}
           .put(BinaryType.get().toString(), BinaryType.get())
           .put(UnknownType.get().toString(), UnknownType.get())
           .put(VariantType.get().toString(), VariantType.get())
+          .put(GeometryType.get().toString(), GeometryType.get())
+          .put(GeographyType.get().toString(), GeographyType.get())
           .buildOrThrow();
 
   private static final Pattern FIXED = Pattern.compile("fixed\\[\\s*(\\d+)\\s*\\]");
@@ -566,11 +568,12 @@ public static class GeometryType extends PrimitiveType {
     private final String crs;
 
     private GeometryType(String crs) {
+      Preconditions.checkNotNull(crs, "CRS cannot be null");
       this.crs = crs;
     }
 
     public static GeometryType get() {
-      return of("");
+      return new GeometryType("");
     }
 
     public static GeometryType of(String crs) {
@@ -615,12 +618,13 @@ public static class GeographyType extends PrimitiveType {
     private final EdgeInterpolationAlgorithm algorithm;
 
     private GeographyType(String crs, EdgeInterpolationAlgorithm algorithm) {
+      Preconditions.checkNotNull(crs, "CRS cannot be null");
       this.crs = crs;
       this.algorithm = algorithm;
     }
 
     public static GeographyType get() {
-      return of("");
+      return new GeographyType("", null);
     }
 
     public static GeographyType of(String crs) {

diff --git a/api/src/test/java/org/apache/iceberg/TestSchema.java b/api/src/test/java/org/apache/iceberg/TestSchema.java
@@ -29,6 +29,7 @@
 import java.util.stream.Stream;
 import org.apache.iceberg.expressions.Literal;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.types.EdgeInterpolationAlgorithm;
 import org.apache.iceberg.types.Type;
 import org.apache.iceberg.types.Types;
 import org.junit.jupiter.api.Test;
@@ -43,7 +44,11 @@ public class TestSchema {
       ImmutableList.of(
           Types.TimestampNanoType.withoutZone(),
           Types.TimestampNanoType.withZone(),
-          Types.VariantType.get());
+          Types.VariantType.get(),
+          Types.GeometryType.get(),
+          Types.GeometryType.of("srid:3857"),
+          Types.GeographyType.get(),
+          Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY));
 
   private static final Schema INITIAL_DEFAULT_SCHEMA =
       new Schema(

diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java b/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java
@@ -658,7 +658,11 @@ private static Stream<Arguments> testTypes() {
         Arguments.of(Types.UnknownType.get()),
         Arguments.of(Types.VariantType.get()),
         Arguments.of(Types.TimestampNanoType.withoutZone()),
-        Arguments.of(Types.TimestampNanoType.withZone()));
+        Arguments.of(Types.TimestampNanoType.withZone()),
+        Arguments.of(Types.GeometryType.get()),
+        Arguments.of(Types.GeometryType.of("srid:3857")),
+        Arguments.of(Types.GeographyType.get()),
+        Arguments.of(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)));
   }
 
   @ParameterizedTest

diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypes.java b/api/src/test/java/org/apache/iceberg/types/TestTypes.java
@@ -115,7 +115,7 @@ public void testNestedFieldBuilderIdCheck() {
 
     assertThatExceptionOfType(IllegalArgumentException.class)
         .isThrownBy(() -> Types.fromPrimitiveString("geography(srid:4269, BadAlgorithm)"))
-        .withMessageContaining("Invalid edge interpolation algorithm name")
+        .withMessageContaining("Invalid edge interpolation algorithm")
         .withMessageContaining("BadAlgorithm");
 
     // Test geography type with various spacing

diff --git a/core/src/main/java/org/apache/iceberg/SchemaParser.java b/core/src/main/java/org/apache/iceberg/SchemaParser.java
@@ -314,13 +314,13 @@ private static Types.MapType mapFromJson(JsonNode json) {
   }
 
   private static Types.GeometryType geometryFromJson(JsonNode json) {
-    String crs = JsonUtil.getStringOrNull("crs", json);
+    String crs = JsonUtil.getStringOrNull(CRS, json);
     return Types.GeometryType.of(crs);
   }
 
   private static Types.GeographyType geographyFromJson(JsonNode json) {
-    String crs = JsonUtil.getStringOrNull("crs", json);
-    String algorithm = JsonUtil.getStringOrNull("algorithm", json);
+    String crs = JsonUtil.getStringOrNull(CRS, json);
+    String algorithm = JsonUtil.getStringOrNull(ALGORITHM, json);
     return Types.GeographyType.of(crs, algorithm);
   }
 

diff --git a/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java b/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java
@@ -29,6 +29,8 @@ public class GeometryUtil {
 
   private GeometryUtil() {}
 
+  private static final int DEFAULT_DIMENSION = 2;
+
   public static byte[] toWKB(Geometry geom) {
     WKBWriter wkbWriter = new WKBWriter(getOutputDimension(geom), false);
     return wkbWriter.write(geom);
@@ -58,7 +60,7 @@ public static Geometry fromWKT(String wkt) {
   }
 
   private static int getOutputDimension(Geometry geom) {
-    int dimension = 2;
+    int dimension = DEFAULT_DIMENSION;
     Coordinate coordinate = geom.getCoordinate();
 
     // We need to set outputDimension = 4 for XYM geometries to make JTS WKTWriter or WKBWriter work

diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java b/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java
@@ -27,6 +27,7 @@
 import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.iceberg.expressions.Literal;
 import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.EdgeInterpolationAlgorithm;
 import org.apache.iceberg.types.Type;
 import org.apache.iceberg.types.Types;
 import org.apache.iceberg.types.Types.BinaryType;
@@ -36,6 +37,8 @@
 import org.apache.iceberg.types.Types.DoubleType;
 import org.apache.iceberg.types.Types.FixedType;
 import org.apache.iceberg.types.Types.FloatType;
+import org.apache.iceberg.types.Types.GeographyType;
+import org.apache.iceberg.types.Types.GeometryType;
 import org.apache.iceberg.types.Types.IntegerType;
 import org.apache.iceberg.types.Types.ListType;
 import org.apache.iceberg.types.Types.LongType;
@@ -71,7 +74,11 @@ private static List<? extends Type> primitiveTypes() {
         VariantType.get(),
         UnknownType.get(),
         TimestampNanoType.withoutZone(),
-        TimestampNanoType.withZone());
+        TimestampNanoType.withZone(),
+        GeometryType.get(),
+        GeometryType.of("srid:3857"),
+        GeographyType.get(),
+        GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY));
   }
 
   private static NestedField[] primitiveFields(