szehon-ho commented on code in PR #16851:
URL: https://github.com/apache/iceberg/pull/16851#discussion_r3431881346
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/TestSparkSchemaUtil.java:
##########
@@ -93,6 +98,40 @@ public void testSchemaConversionWithMetaDataColumnSchema() {
}
}
+ @Test
+ public void testGeospatialTypeConversion() {
+ Types.GeometryType geometry = Types.GeometryType.of("EPSG:3857");
+ DataType sparkGeometry = SparkSchemaUtil.convert(geometry);
+ assertThat(sparkGeometry).isInstanceOf(GeometryType.class);
+ assertThat(((GeometryType) sparkGeometry).crs()).isEqualTo("EPSG:3857");
+ assertThat(SparkSchemaUtil.convert(sparkGeometry)).isEqualTo(geometry);
+
+ Types.GeographyType geography = Types.GeographyType.of("EPSG:4326");
+ DataType sparkGeography = SparkSchemaUtil.convert(geography);
+ assertThat(sparkGeography).isInstanceOf(GeographyType.class);
+ assertThat(((GeographyType) sparkGeography).crs()).isEqualTo("EPSG:4326");
+ assertThat(SparkSchemaUtil.convert(sparkGeography)).isEqualTo(geography);
Review Comment:
Coverage here only exercises the default `SPHERICAL` algorithm, so the
algorithm-handling decision above isn't tested. Worth adding a geography with a
non-default algorithm (e.g. `VINCENTY`) asserting either round-trip
preservation or the thrown exception, depending on how the conversion handles
it.
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/TypeToSparkType.java:
##########
@@ -145,6 +147,12 @@ public DataType primitive(Type.PrimitiveType primitive) {
return BinaryType$.MODULE$;
case BINARY:
return BinaryType$.MODULE$;
+ case GEOMETRY:
+ Types.GeometryType geometry = (Types.GeometryType) primitive;
+ return GeometryType$.MODULE$.apply(geometry.crs());
+ case GEOGRAPHY:
+ Types.GeographyType geography = (Types.GeographyType) primitive;
+ return GeographyType$.MODULE$.apply(geography.crs());
Review Comment:
This silently drops the edge-interpolation algorithm. Iceberg's
`GeographyType` carries an `EdgeAlgorithm` (`SPHERICAL, VINCENTY, THOMAS,
ANDOYER, KARNEY`), but only the CRS is forwarded here.
`GeographyType.apply(crs)` defaults to `SPHERICAL`, and Spark only supports
`SPHERICAL` — so a non-spherical Iceberg geography is silently coerced rather
than reported.
This file's convention is to fail loudly on unsupported mappings (cf. `case
TIME` -> "Spark does not support time fields"). Consider rejecting
non-spherical algorithms with a clear `UnsupportedOperationException` instead
of coercing. WDYT?
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/PruneColumnsWithoutReordering.java:
##########
@@ -244,6 +246,8 @@ public Type primitive(Type.PrimitiveType primitive) {
.put(TypeID.STRING, ImmutableSet.of(StringType$.class))
.put(TypeID.FIXED, ImmutableSet.of(BinaryType$.class))
.put(TypeID.BINARY, ImmutableSet.of(BinaryType$.class))
+ .put(TypeID.GEOMETRY, ImmutableSet.of(GeometryType.class))
+ .put(TypeID.GEOGRAPHY, ImmutableSet.of(GeographyType.class))
Review Comment:
Unlike `DECIMAL`, geo types get no additional CRS/SRID compatibility check
in `primitive()`. Since `primitive()` returns the table's own type there's no
corruption, but a requested geo column with a mismatched CRS passes pruning
silently. Is deferring SRID validation intentional (consistent with
UUID->String), or worth a follow-up?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
