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.

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.

Thank you

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.

Nit: use the constant for "type"

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

Nit: use switch statement like rest of the code.

Refactored to use switch-case.

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

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.

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.

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

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.

👍

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

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.

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.

Precondition, check null and throw exception?

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

Nit: can remove redundant 'name'

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

why not , new GeometryType("")

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

why not new GeographyType("")

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

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.

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

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

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.

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.