Hive: Map geometry and geography type to binary in HiveSchemaUtil#17112
Hive: Map geometry and geography type to binary in HiveSchemaUtil#17112csringhofer wants to merge 1 commit into
Conversation
| 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)); |
There was a problem hiding this comment.
add a .describedAs("hive schema %s", schema) before the .containsExactly() for even better assertion details.
There was a problem hiding this comment.
I think that's just noise, don't see the additional value.
| return "timestamp"; | ||
| case FIXED: | ||
| case BINARY: | ||
| case GEOMETRY: |
There was a problem hiding this comment.
as noted by the automated review, consider moving this switch statement to the java17
case key -> value style. The codebase moved to it entirely last month, then moved back to ease cherrypicking; if you look at the file history you can find the diff so see what to bring over, just for this switch
There was a problem hiding this 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?
There was a problem hiding this 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.
gaborkaszab
left a comment
gaborkaszab
left a comment
There was a problem hiding this comment.
Hey @csringhofer ,
Thanks for the PR! I took a quick look, but I'm not an expert on this area. Maybe @pvary ?
| 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)); |
There was a problem hiding this comment.
I think that's just noise, don't see the additional value.
| return "timestamp"; | ||
| case FIXED: | ||
| case BINARY: | ||
| case GEOMETRY: |
There was a problem hiding this 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?
| @Test | ||
| public void testGeometryTypeWithCustomCrsConvertToHiveSchema() { | ||
| Schema schema = new Schema(optional(0, "geometry_field", Types.GeometryType.of("EPSG:3857"))); | ||
| List<FieldSchema> hiveSchema = HiveSchemaUtil.convert(schema); |
There was a problem hiding this 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?
There was a problem hiding this comment.
Yes, this is intentional because Hive metastore has no 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
| } | ||
|
|
||
| @Test | ||
| public void testGeographyTypeWithCustomCrsConvertToHiveSchema() { |
There was a problem hiding this comment.
I think the test below with both CRS and algorithm is enough.
There was a problem hiding this comment.
Will deal with this test once we resolve discussion on testGeometryTypeWithCustomCrsConvertToHiveSchema
| } | ||
|
|
||
| @Test | ||
| public void testGeometryTypeWithCustomCrsConvertToHiveSchema() { |
There was a problem hiding this comment.
Is this test any different than the one above? It has a custom CRS, but doesn't seem to bring any extra test coverage.
There was a problem hiding this 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.
| return "timestamp"; | ||
| case FIXED: | ||
| case BINARY: | ||
| case GEOMETRY: |
There was a problem hiding this comment.
Since variant doesn't have an HMS representation either, and it maps to unknown, shouldn't we follow that pattern and map geoXY types to variant too?
3 participants
Add GEOMETRY and GEOGRAPHY cases to HiveSchemaUtil.convertToTypeString, mapping them to "binary".
"binary" was chosen instead of "unknown" because geometry/geography are WKB-encoded byte arrays - older readers not familiar with Iceberg/Parquet GEOMETRY can still read the created Parquet files by reading the column as BINARY and converting from WKB to their representation (st_GeomFromWKB() in Hive).
Motivation:
Bumped into this while integrating Iceberg geometry support into Apache Impala, which uses the Hive catalog for tests. Creating a table with a geometry column would throw UnsupportedOperationException from the default
branch of the switch. By patching HiveSchemaUtil in Iceberg I as able to create tables with GEOMETRY columns.