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

        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


  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


  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.

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

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

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

      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.

So we consider CRS to be case sensitive?

The capture group of a case insensitive regular expression returns the matched characters without modification. So if CRS is case sensitive, you can still use a regular expression, like this:

Pattern pattern = Pattern.compile("geometry\\(([^)]*)\\)", Pattern.CASE_INSENSITIVE);

This test will pass:

Matcher m = pattern.matcher("GEOMETRY(aBc)"); assertThat(m.matches()).isTrue(); assertThat(m.group(1)).isEqualTo("aBc");

I'd prefer using a case insensitive pattern on typeString like this, rather than startsWith and substring with a hard-coded length.

Switched to using case insensitive regex pattern matching.

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.

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

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

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

 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.

The empty string should not be allowed. That is an invalid CRS because it has no information. Null is fine to support here if you expect to have situations in which callers will find it convenient to pass null to mean "use the default".

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?

Added precondition with error message: Invalid edge interpolation algorithm: null.

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'

Fixed. Now the error message is Invalid edge interpolation algorithm: %s

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("")

Fixed. Now we are using null as default value as requested. For now it is new GeometryType(null).

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("")

Fixed. Now it is new GeographyType(null, null).

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

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

        .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

In this suite, these cases would be added to fromPrimitiveString and fromTypeName.

Moved tests to fromTypeName and fromPrimitiveString.

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

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

  }

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


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

This is reverted since we are not encoding geospatial types as JSON.

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

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?

I think it would be easier to allow null to indicate the default. That's easier to detect for toString to show geometry instead of geometry().

We allow null and not allow empty string for CRS. Additionally we check that the CRS should not have leading or trailing spaces to make sure that the value of CRS will be persisted after converting to string and parsing it back.

Curious, why not always have toString show the default value like Geography? Geometry("OGC:CRS84"). Is it to save some bytes ? Seems to be easier imo.

Geography does not always show the default CRS. It happens when the edge algorithm is not using default value but CRS is left unset. We have to fill in the default CRS as the first argument in this case.

I thought that the behavior of toString is to omit optional parameters when possible. See https://github.com/apache/iceberg/pull/12346/files/0c451cac53f4098680aa0ae518af70dc45efaeb1#r1994430054.

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?

This file is removed as we don't need WKB/WKT serialization according to the latest spec.

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()?

… review comments

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

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

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

core/src/test/java/org/apache/iceberg/data/DataTest.java

rdblue · 2025-03-21T21:00:16Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java

      case FIXED:
      case BINARY:
+      case GEOMETRY:
+      case GEOGRAPHY:


Why is this included? I don't think that we want to translate these into Hive types.

I've reverted this change. Does it mean that Hive catalog won't support geospatial columns?

When this is needed, we can add it. But there's no value in translating to binary right now.

rdblue · 2025-03-25T23:04:33Z

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

+    Matcher geometry = GEOMETRY_PARAMETERS.matcher(typeString);
+    if (geometry.matches()) {
+      String crs = geometry.group(1);
+      Preconditions.checkArgument(!crs.contains(","), "Invalid CRS: %s", crs);


Why is this invalid? This should pass the string as-is. It is up to others to determine what is a valid CRS.

Comma in CRS will disrupt the parsing of geography types such as geography(crs,with,comma, spherical), so I treat crs containing comma as invalid to avoid ambiguity.

I'll remove this check and pass the CRS string as-is, since it won't cause ambiguity for geometry types.

rdblue · 2025-03-25T23:05:42Z

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

+    }
+
+    public String crs() {
+      return crs;


If CRS is null, should this return DEFAULT_CRS?

I think it is better to return DEFAULT_CRS, so that the callers don't have to handle both nulls and default CRS.

We can make GeographyType.algorithm return EdgeAlgorithm.SPHERICAL when algorithm is null as well.

rdblue · 2025-03-25T23:09:42Z

Looks great! There are some minor nits, but I think it makes sense to commit this to move forward. Thanks @Kontinuation and @szehon-ho!

szehon-ho · 2025-03-25T23:26:05Z

Thanks , great work @Kontinuation , and thanks @rdblue !

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

Merged

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

api/src/main/java/org/apache/iceberg/types/Types.java Outdated Show resolved Hide resolved

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

Renamed GeographyType.of(crs) to GeographyType.forCRS, addressed more…

7c8fa2b

… review comments

sfc-gh-msaecker mentioned this pull request Mar 20, 2025

Backport geometry and geography types support Snowflake-Labs/iceberg#12

Closed