huan233usc commented on code in PR #16851:
URL: https://github.com/apache/iceberg/pull/16851#discussion_r3433233412
##########
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:
Agreed. Fixed in `2f239945f` to fail loudly for non-`SPHERICAL` geography
algorithms instead of silently coercing to Spark geography.
##########
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:
Added coverage in `2f239945f` with a `VINCENTY` geography asserting the
unsupported conversion exception.
##########
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:
Good point. I am leaving CRS/SRID validation out of this split for now:
pruning only validates the requested Spark type class and returns the table
Iceberg type, so it cannot change the table CRS. The CRS/SRID compatibility
check belongs with the read/write path where Spark values are materialized.
##########
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:
Agreed. Fixed in `2f239945f` to fail loudly for non-`SPHERICAL` geography
algorithms instead of silently coercing to Spark geography.
##########
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:
Added coverage in `2f239945f` with a `VINCENTY` geography asserting the
unsupported conversion exception.
##########
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:
Good point. I am leaving CRS/SRID validation out of this split for now:
pruning only validates the requested Spark type class and returns the table
Iceberg type, so it cannot change the table CRS. The CRS/SRID compatibility
check belongs with the read/write path where Spark values are materialized.
--
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]
