csringhofer commented on code in PR #17112:
URL: https://github.com/apache/iceberg/pull/17112#discussion_r3535281627
##########
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:
Yes, this is intentional because Hive metastore has GEOMETRY/GEOGRAPHY type,
so also can't store these extra parameters. In Impala the source of truth for
the schema is Iceberg, the table in HMS is only used of its Iceberg related
properties. This is also how we plan to handled VARIANT. It is currently
mapped to "unknown", which would be also a possibility in this patch, but I
think that "binary" is generic enough - a byte array and we don't claim
anything about its content.
For other systems: I checked Impala interop only with Trino. AFAIK Trino
reimplemented some parts of Hive catalog, including HiveSchemaUtil. They map
geometry to binary here:
https://github.com/trinodb/trino/blob/bfe1a8e05dead169fcc6400c903e39d3379e52c5/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/util/HiveSchemaUtil.java#L46
##########
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() {
Review Comment:
This test has no benefit now, but IMO it is a useful placeholder in case HMS
adds GEOMETRY/GEOGRAPHY - if this file will be updated to map to GEOMETRY, then
checking what happens with custom CRS becomes important. I can merge this to
testGeometryTypeConvertToHiveSchema to cause less noise, or probably it is
enough to add a comment there to also test CRS/edge interpolation in case the
test gets updated.
##########
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:
Is it ok do this in another patch? I would prefer to not mix logic change
with refactor, but can switch style of course if needed.
##########
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);
+ assertThat(hiveSchema).containsExactly(new FieldSchema("geometry_field",
"binary", null));
+ }
+
+ @Test
+ public void testGeographyTypeConvertToHiveSchema() {
+ Schema schema = new Schema(optional(0, "geography_field",
Types.GeographyType.crs84()));
+ List<FieldSchema> hiveSchema = HiveSchemaUtil.convert(schema);
+ assertThat(hiveSchema).containsExactly(new FieldSchema("geography_field",
"binary", null));
+ }
+
+ @Test
+ public void testGeographyTypeWithCustomCrsConvertToHiveSchema() {
Review Comment:
Will deal with this test once we resolve discussion on
testGeometryTypeWithCustomCrsConvertToHiveSchema
--
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]
