API, Core: Add geometry and geography types support #12346

Kontinuation · 2025-02-20T07:03:45Z

This adds 2 primitive types to iceberg-api and iceberg-core for supporting geospatial data types, partially implementing the iceberg geo spec: #10981

The newly added primitive types are:

geometry(C): geometries with linear/planar edge interpolation
geography(C, A): geometries with non-linear edge interpolation, the algorithm for interpolating edges is parameterized by A.

jiayuasu · 2025-02-22T22:15:07Z

@rdblue @szehon-ho @flyrain please review when you have time 🙏🏻

szehon-ho

Hey , really sorry for delay, let's work on it this week. Some early comments

szehon-ho · 2025-02-26T19:45:07Z

api/src/main/java/org/apache/iceberg/Geography.java

+
+  private final Geometry geometry;
+
+  public Geography(Geometry geometry) {


Why do we need interop between Geography and Geometry? I assume we have just have a class for each.

We define Geography this way to reuse the data structures of coordinates and geospatial objects (Points, LineString, etc.), as well as WKB/WKT functions provided by JTS. Geography does not interoperate with Geometry, they are different classes but have the same data structure.

Can we have a base class, and two implementing classes then? its confusing as it is.

I'll try removing Geometry and Geography from iceberg-api, and use ByteBuffer as the underlying Java class for these types.

szehon-ho · 2025-02-26T19:46:32Z

api/src/main/java/org/apache/iceberg/expressions/BoundLiteralPredicate.java

@@ -88,11 +91,64 @@ public boolean test(T value) {
        return String.valueOf(value).startsWith((String) literal.value());
      case NOT_STARTS_WITH:
        return !String.valueOf(value).startsWith((String) literal.value());
+      case ST_INTERSECTS:


Lets split out predicate pruning support. I feel 80% of the changes is to support those, let's focus in this pr to get the API right.

Predicates and pruning were removed from this PR.

szehon-ho · 2025-03-04T20:00:57Z

build.gradle

@@ -292,6 +292,7 @@ project(':iceberg-api') {

  dependencies {
    implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+    api libs.jts.core


I am not comfortable leaking the JTS API from Iceberg API, as this forces all the consumer to now depend on JTS. Can we instead encapsulate this dep in iceberg-core? Going to check with other Iceberg PMC's on this.

At very least this should be implementation.

szehon-ho · 2025-03-04T20:01:38Z

core/src/main/java/org/apache/iceberg/SchemaParser.java

+    if (primitive.typeId() == Type.TypeID.GEOMETRY) {
+      Types.GeometryType geometryType = (Types.GeometryType) primitive;
+      generator.writeStartObject();
+      generator.writeStringField("type", "geometry");


Nit: use the constant for "type"

Replaced constant literals such as "type", "geometry", "geography", "crs" and "algorithm" with constants.

szehon-ho · 2025-03-04T20:02:15Z

core/src/main/java/org/apache/iceberg/SchemaParser.java

@@ -141,11 +143,34 @@ static void toJson(Types.MapType map, JsonGenerator generator) throws IOExceptio
  }

  static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws IOException {
-    generator.writeString(primitive.toString());
+    if (primitive.typeId() == Type.TypeID.GEOMETRY) {


Nit: use switch statement like rest of the code.

Refactored to use switch-case.

szehon-ho · 2025-03-04T20:03:06Z

core/src/main/java/org/apache/iceberg/SchemaParser.java

+      generator.writeStringField("type", "geometry");
+      String crs = geometryType.crs();
+      if (!crs.isEmpty()) {
+        generator.writeStringField("crs", geometryType.crs());


Nit: "crs" seems used frequently enough to warrant a constant.

szehon-ho · 2025-03-04T20:04:57Z

api/src/main/java/org/apache/iceberg/types/TypeUtil.java

@@ -535,6 +535,8 @@ private static int estimateSize(Type type) {
        return ((Types.FixedType) type).length();
      case BINARY:
      case VARIANT:
+      case GEOMETRY:
+      case GEOGRAPHY:


can we put a comment how we arrived at 80?

We cannot give an accurate estimation without statistics of the data, so I've simply reused the number for BINARY and VARIANT.

80 is roughly size of a polygon or linestring with 4 coordinates, which denotes a box. This is larger for a dataset full of points and smaller for datasets containing complex shapes, but I think it is a reasonable guess.

Ok I meant code comment :)

I've added reasons why we use 80 (even though the choice was made randomly like https://github.com/apache/iceberg/pull/11324/files#r1821277533) and avoid falling back into the same case as BINARY and VARIANT.

szehon-ho · 2025-03-04T20:07:49Z

api/src/main/java/org/apache/iceberg/types/Types.java

@@ -70,6 +75,20 @@ public static Type fromTypeName(String typeString) {
      return TYPES.get(lowerTypeString);
    }

+    if (lowerTypeString.startsWith("geometry")) {


Why cant we put the startsWith in the regex, like DECIMAL and FIXED

We cannot match the regex with lowerTypeString, because we want to extract the original CRS from the type instead of a lower case one. I'll see how to make it more concise and get rid of startsWith.

szehon-ho · 2025-03-05T00:35:43Z

api/src/main/java/org/apache/iceberg/expressions/Literal.java

+    return new Literals.GeometryLiteral(value);
+  }
+
+  static Literal<Geography> of(Geography value) {


I guess here, let's specifically not leak the Geography in the API, we should just have a wrapper class as well.

I'll remove GeometryLiteral ad GeographyLiteral. The iceberg expressions should only work with bounding boxes, so there's no need to involve full-fledged Geometry and Geography objects.

szehon-ho · 2025-03-05T00:35:58Z

api/src/main/java/org/apache/iceberg/Geography.java

+
+  private final Geometry geometry;
+
+  public Geography(Geometry geometry) {


Can we have a base class, and two implementing classes then? its confusing as it is.

szehon-ho · 2025-03-05T00:53:44Z

api/src/main/java/org/apache/iceberg/types/Types.java

+    }
+
+    public static GeographyType of(String crs) {
+      return of(crs, DEFAULT_ALGORITHM);


another option is to allow null for algorithm right? and the code can default it , as per the spec?

Fixed. The algorithm field is null when no algorithm is specified.

…ort spec for geospatial types

…ry and geography types to ByteBuffer

Kontinuation · 2025-03-05T07:22:02Z

I have removed dependency to JTS from iceberg-api, now the underlying Java type of Geometry and Geography are ByteBuffer. Now JTS is a implementation dependency of iceberg-core.

I have removed code for converting between ByteBuffer and geospatial types, removed GeometryLiteral/GeographyLiteral, as well as default initial/write value support for geospatial fields. I'll proceed adding them once we are all agree on the API of primitive geospatial types.

hsiang-c · 2025-03-05T22:53:46Z

api/src/main/java/org/apache/iceberg/types/Types.java

+      Matcher geography = GEOGRAPHY_PARAMETERS.matcher(typeString.substring(9));
+      if (geography.matches()) {
+        return GeographyType.of(geography.group(1), geography.group(2));
+      }


In both geometry and geography cases, do you think we should throw IllegalArgumentException when patterns don't match?

We'll throw an IllegalArgumentException at the bottom of this function if none of the pattern matches: "Cannot parse type string to primitive: " + typeString. I think this is already informative. Do you think that we need a more specific error message for geospatial types?

No worries, I didn't pay attention to the very bottom. LGTM.

hsiang-c · 2025-03-05T23:10:46Z

api/src/main/java/org/apache/iceberg/transforms/Identity.java

 import org.apache.iceberg.types.Type;
 import org.apache.iceberg.util.SerializableFunction;

 class Identity<T> implements Transform<T, T> {
  private static final Identity<?> INSTANCE = new Identity<>();

+  private static final Set<Type.TypeID> UNSUPPORTED_TYPES =


hsiang-c · 2025-03-05T23:15:34Z

api/src/main/java/org/apache/iceberg/types/Types.java

+    }
+
+    public static GeometryType of(String crs) {
+      return new GeometryType(crs == null ? "" : crs);


(nit) Maybe crs == null can be crs == null || crs.isEmpty()

I believed that including crs.isEmpty() was redundant in this situation, so I didn't add it, as we would return an empty string regardless.

hsiang-c · 2025-03-05T23:37:31Z

core/src/main/java/org/apache/iceberg/util/GeometryUtil.java

+    // We need to set outputDimension = 4 for XYM geometries to make JTS WKTWriter or WKBWriter work
+    // correctly.
+    // The WKB/WKT writers will ignore Z ordinate for XYM geometries.
+    if (!Double.isNaN(coordinate.getZ())) {


Perhaps we could do (not a must).

if (Coordinate.NULL_ORDINATE != coordinate.getZ())

I'm afraid that we cannot do this. Coordinate.NULL_ORDINATE is NaN, comparing it with other floating point number (including NaN) using != will always yield true.

hsiang-c · 2025-03-05T23:45:49Z

core/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java

+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.GeometryFactory;
+
+public class TestGeometryUtil {


It seems like you covered Coordinate and its subclasses except ExtendedCoordinate, am I right?

Yes. As far as I know, ExtendedCoordinate is part of JTS example code so we don't need to handle it. JTS will only be used internally in iceberg and it exchanges geospatial data with other systems through WKB/Bounds byte buffer, so there's no need to take non-standard extensions of Coordinate into consideration.

Thanks for the explanation.

hsiang-c

LGTM

szehon-ho

Thanks @Kontinuation for removing the api dependency on jts from iceberg-api, this looks a lot closer to me

szehon-ho · 2025-03-07T01:06:24Z

api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java

+  }
+
+  public static EdgeInterpolationAlgorithm fromName(String algorithmName) {
+    try {


Precondition, check null and throw exception?

szehon-ho · 2025-03-07T01:07:05Z

api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java

+      return EdgeInterpolationAlgorithm.valueOf(algorithmName.toUpperCase(Locale.ENGLISH));
+    } catch (IllegalArgumentException e) {
+      throw new IllegalArgumentException(
+          String.format("Invalid edge interpolation algorithm name: %s", algorithmName), e);


Nit: can remove redundant 'name'

szehon-ho · 2025-03-07T01:08:48Z

api/src/main/java/org/apache/iceberg/types/Types.java

+    }
+
+    public static GeometryType get() {
+      return of("");


why not , new GeometryType("")

szehon-ho · 2025-03-07T01:09:36Z

api/src/main/java/org/apache/iceberg/types/Types.java

+    }
+
+    public static GeographyType get() {
+      return of("");


why not new GeographyType("")

szehon-ho · 2025-03-07T01:10:34Z

api/src/test/java/org/apache/iceberg/types/TestTypes.java

@@ -90,5 +90,42 @@ public void testNestedFieldBuilderIdCheck() {
    assertThatExceptionOfType(NullPointerException.class)
        .isThrownBy(() -> required("field").ofType(Types.StringType.get()).build())
        .withMessage("Id cannot be null");
+
+    assertThat(Types.fromPrimitiveString("geometry")).isEqualTo(Types.GeometryType.get());


this should be in its own test

szehon-ho · 2025-03-07T01:11:07Z

core/src/main/java/org/apache/iceberg/SchemaParser.java

@@ -277,6 +313,17 @@ private static Types.MapType mapFromJson(JsonNode json) {
    }
  }

+  private static Types.GeometryType geometryFromJson(JsonNode json) {
+    String crs = JsonUtil.getStringOrNull("crs", json);


nit: these (and following) can be replaced by the constants?

szehon-ho · 2025-03-07T01:17:06Z

api/src/main/java/org/apache/iceberg/types/Types.java

+      return TypeID.GEOMETRY;
+    }
+
+    public String crs() {


maybe can annotate NotNull?

The NotNull annotation requires an additional dependency, and I noticed that it is not currently used in iceberg-api or iceberg-core. Would you like me to add the additional dependency and annotate this method?

Got it, nvm it may not be worth it

szehon-ho · 2025-03-07T01:17:27Z

api/src/main/java/org/apache/iceberg/types/Types.java

+    private final String crs;
+
+    private GeometryType(String crs) {
+      this.crs = crs;


Precondition that crs is not null?

szehon-ho · 2025-03-07T01:19:02Z

core/src/main/java/org/apache/iceberg/util/GeometryUtil.java

+  }
+
+  private static int getOutputDimension(Geometry geom) {
+    int dimension = 2;


nit: make a constant to avoid instantiating it every time?

szehon-ho · 2025-03-07T01:30:35Z

Also, (as can't comment on files that are not in the change)

Do we need to add Geo types to following places?

Types.java TYPES constant?
TestSchemaUnionByFieldName primitiveTypes()?
TestSchema TEST_TYPES?
TestTypesUtil testTypes()?

github-actions bot added API core build labels Feb 20, 2025

Kontinuation force-pushed the pr-geo-api-core branch from b39e32c to 904f503 Compare February 20, 2025 07:09

Kontinuation mentioned this pull request Feb 20, 2025

WIP Parquet: Support reading/writing geometry and geography columns #12347

Draft

Kontinuation marked this pull request as ready for review February 20, 2025 08:15

Kontinuation force-pushed the pr-geo-api-core branch from 904f503 to 896bc19 Compare February 22, 2025 03:59

jiayuasu mentioned this pull request Feb 24, 2025

Fix writing XYM geometries as WKB locationtech/jts#1092

Open

szehon-ho reviewed Mar 3, 2025

View reviewed changes

Kontinuation force-pushed the pr-geo-api-core branch from 896bc19 to 4d1bb83 Compare March 4, 2025 08:27

szehon-ho reviewed Mar 5, 2025

View reviewed changes

Kontinuation added 4 commits March 5, 2025 11:30

add geometry and geography types to iceberg-api and iceberg-core

d9274ee

Remove spatial predicate evaluation code from GeometryUtil

cccb563

Fix interpretation of default CRS, disallow identity, bucketing and s…

8c1c6d5

…ort spec for geospatial types

Fix review issues

73f6022

Kontinuation force-pushed the pr-geo-api-core branch from 4d1bb83 to 73f6022 Compare March 5, 2025 04:07

Remove Geometry and Geography objects from iceberg-api and map geomet…

7bbfb8b

…ry and geography types to ByteBuffer

Comment the estimated size of geometry and geography objects

c3b57f7

hsiang-c reviewed Mar 5, 2025

View reviewed changes

hsiang-c approved these changes Mar 6, 2025

View reviewed changes

szehon-ho reviewed Mar 7, 2025

View reviewed changes

Fixed problems according to review comments

eb1896a

Kontinuation force-pushed the pr-geo-api-core branch from fc193c6 to eb1896a Compare March 10, 2025 02:35

Kontinuation requested a review from szehon-ho March 10, 2025 03:53


		private final Geometry geometry;

		public Geography(Geometry geometry) {

API, Core: Add geometry and geography types support #12346

Are you sure you want to change the base?

API, Core: Add geometry and geography types support #12346

Conversation

Kontinuation commented Feb 20, 2025 • edited Loading

jiayuasu commented Feb 22, 2025

szehon-ho 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

szehon-ho Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szehon-ho Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kontinuation Mar 5, 2025 • edited Loading

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

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

szehon-ho Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kontinuation commented Mar 5, 2025 • edited Loading

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

Kontinuation Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

hsiang-c Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Kontinuation Mar 6, 2025 • edited Loading

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

hsiang-c left a comment

Choose a reason for hiding this comment

szehon-ho 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

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

szehon-ho commented Mar 7, 2025

Kontinuation commented Feb 20, 2025 •

edited

Loading

szehon-ho Feb 26, 2025 •

edited

Loading

szehon-ho Mar 4, 2025 •

edited

Loading

Kontinuation Mar 5, 2025 •

edited

Loading

szehon-ho Mar 5, 2025 •

edited

Loading

Kontinuation commented Mar 5, 2025 •

edited

Loading

Kontinuation Mar 6, 2025 •

edited

Loading

hsiang-c Mar 5, 2025 •

edited

Loading

Kontinuation Mar 6, 2025 •

edited

Loading