gaborkaszab commented on code in PR #17112:
URL: https://github.com/apache/iceberg/pull/17112#discussion_r3534675144
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveSchemaUtil.java:
##########
@@ -212,6 +213,44 @@ public void testVariantTypeConvertToHiveSchema() {
assertThat(hiveSchema).containsExactly(new FieldSchema("variant_field",
"unknown", null));
}
+ @Test
+ public void testGeometryTypeConvertToHiveSchema() {
+ Schema schema = new Schema(optional(0, "geometry_field",
Types.GeometryType.crs84()));
+ List<FieldSchema> hiveSchema = HiveSchemaUtil.convert(schema);
+ assertThat(hiveSchema).containsExactly(new FieldSchema("geometry_field",
"binary", null));
+ }
+
+ @Test
+ public void testGeometryTypeWithCustomCrsConvertToHiveSchema() {
+ Schema schema = new Schema(optional(0, "geometry_field",
Types.GeometryType.of("EPSG:3857")));
+ List<FieldSchema> hiveSchema = HiveSchemaUtil.convert(schema);
Review Comment:
A SerDe conversion from Iceberg schema to Hive schema and then back to
Iceberg schema seems lossy with Geometry and Geography types. We'd loose CRS
and the ALGORITHM. Isn't that an issue? Are engines expected to use the Iceberg
schema instead of the Hive schema for these fields?
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveSchemaUtil.java:
##########
@@ -212,6 +213,44 @@ public void testVariantTypeConvertToHiveSchema() {
assertThat(hiveSchema).containsExactly(new FieldSchema("variant_field",
"unknown", null));
}
+ @Test
+ public void testGeometryTypeConvertToHiveSchema() {
+ Schema schema = new Schema(optional(0, "geometry_field",
Types.GeometryType.crs84()));
+ List<FieldSchema> hiveSchema = HiveSchemaUtil.convert(schema);
+ assertThat(hiveSchema).containsExactly(new FieldSchema("geometry_field",
"binary", null));
Review Comment:
I think that's just noise, don't see the additional value.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java:
##########
@@ -166,6 +166,8 @@ private static String convertToTypeString(Type type) {
return "timestamp";
case FIXED:
case BINARY:
+ case GEOMETRY:
Review Comment:
+1 recently we decided to gradually rewrite these switched to the new style
whenever we touch them. Would you mind do the rewrite here?
--
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]
