szehon-ho commented on code in PR #16851:
URL: https://github.com/apache/iceberg/pull/16851#discussion_r3471514633
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/TypeToSparkType.java:
##########
@@ -145,6 +148,16 @@ 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;
+ if (geography.algorithm() != EdgeAlgorithm.SPHERICAL) {
Review Comment:
Nit: consider an explicit translation layer between Iceberg's
`EdgeAlgorithm` and Spark's `EdgeInterpolationAlgorithm` rather than the inline
`!= SPHERICAL` guard. It would make the supported set self-documenting and
could also validate the CRS here (Spark only accepts `OGC:CRS84` for geography,
so other CRS values currently surface a raw Spark error instead of a clear
Iceberg one). Optional.
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/TestSparkSchemaUtil.java:
##########
@@ -93,6 +100,46 @@ 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("OGC:CRS84");
+ DataType sparkGeography = SparkSchemaUtil.convert(geography);
+ assertThat(sparkGeography).isInstanceOf(GeographyType.class);
+ assertThat(((GeographyType) sparkGeography).crs()).isEqualTo("OGC:CRS84");
+ assertThat(SparkSchemaUtil.convert(sparkGeography)).isEqualTo(geography);
+
+
assertThat(SparkSchemaUtil.convert(GeometryType$.MODULE$.apply("EPSG:3857")))
+ .isEqualTo(geometry);
+
assertThat(SparkSchemaUtil.convert(GeographyType$.MODULE$.apply("OGC:CRS84")))
+ .isEqualTo(geography);
+
+ Types.GeographyType vincentyGeography =
+ Types.GeographyType.of("OGC:CRS84", EdgeAlgorithm.VINCENTY);
+ assertThatThrownBy(() -> SparkSchemaUtil.convert(vincentyGeography))
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage("Spark does not support geography edge algorithm:
vincenty");
+ }
+
+ @Test
+ public void testPruneGeospatialTypes() {
+ Schema schema =
+ new Schema(
+ optional(1, "geom", Types.GeometryType.of("EPSG:3857")),
+ optional(2, "geog", Types.GeographyType.of("OGC:CRS84")),
+ optional(3, "id", Types.LongType.get()));
+
+ StructType requestedType = SparkSchemaUtil.convert(schema);
+ Schema pruned = SparkSchemaUtil.prune(schema, requestedType);
+
+ assertThat(pruned.asStruct()).isEqualTo(schema.asStruct());
Review Comment:
Nit: this covers only the happy path. A negative case (requesting an
incompatible Spark type for a geo column, triggering the `Cannot project ...
incompatible type` precondition) would round out coverage, though it mirrors
how other primitives are tested so it's optional.
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/TestSparkSchemaUtil.java:
##########
@@ -93,6 +100,46 @@ public void testSchemaConversionWithMetaDataColumnSchema() {
}
}
+ @Test
+ public void testGeospatialTypeConversion() {
+ Types.GeometryType geometry = Types.GeometryType.of("EPSG:3857");
Review Comment:
Nit: this only exercises explicit CRS values. Adding a default-CRS geometry
round-trip (`Types.GeometryType.crs84()`) would lock in the `null` <->
`OGC:CRS84` normalization, which is the subtlest part of the mapping.
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/SparkTypeToType.java:
##########
@@ -162,6 +164,13 @@ public Type atomic(DataType atomic) {
((DecimalType) atomic).precision(), ((DecimalType) atomic).scale());
} else if (atomic instanceof BinaryType) {
return Types.BinaryType.get();
+ } else if (atomic instanceof GeometryType) {
+ return Types.GeometryType.of(((GeometryType) atomic).crs());
+ } else if (atomic instanceof GeographyType) {
+ // Spark only supports the spherical edge-interpolation algorithm, which
matches the Iceberg
+ // default, so the Spark algorithm is intentionally not propagated here.
Revisit if Spark
+ // starts supporting additional algorithms.
+ return Types.GeographyType.of(((GeographyType) atomic).crs());
Review Comment:
Reverse of the translation-layer suggestion on `TypeToSparkType`: Spark
exposes `algorithm()`, so we could pass it through to Iceberg instead of
dropping it and defaulting to `SPHERICAL`. Functionally identical today (Spark
only has `SPHERICAL`), but keeps both directions symmetric and fails loudly if
Spark ever adds an algorithm Iceberg lacks. The comment already documents the
trade-off, so purely optional.
--
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]
