Skip to content

Hive: Map geometry and geography type to binary in HiveSchemaUtil #17112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
csringhofer wants to merge 1 commit into
base: main
Choose a base branch
from
+41 −0
Open

Hive: Map geometry and geography type to binary in HiveSchemaUtil #17112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@
}

private static String convertToTypeString(Type type) {
switch (type.typeId()) {

Check warning on line 144 in hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java

View workflow job for this annotation

GitHub Actions / check-runtime-deps 

[StatementSwitchToExpressionSwitch] This statement switch can be converted to a new-style arrow switch

Check warning on line 144 in hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java

View workflow job for this annotation

GitHub Actions / build-checks (17) 

[StatementSwitchToExpressionSwitch] This statement switch can be converted to a new-style arrow switch
case BOOLEAN:
return "boolean";
case INTEGER:
Expand All @@ -166,6 +166,8 @@
return "timestamp";
case FIXED:
case BINARY:
case GEOMETRY:

@steveloughran steveloughran Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gaborkaszab gaborkaszab Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 recently we decided to gradually rewrite these switched to the new style whenever we touch them. Would you mind do the rewrite here?

@csringhofer csringhofer Jul 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gaborkaszab Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

case GEOGRAPHY:
return "binary";
case VARIANT:
return "unknown";
Expand Down
39 changes: 39 additions & 0 deletions hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveSchemaUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.apache.iceberg.Schema;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.EdgeAlgorithm;
import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -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));

@steveloughran steveloughran Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a .describedAs("hive schema %s", schema) before the .containsExactly() for even better assertion details.

@gaborkaszab gaborkaszab Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's just noise, don't see the additional value.

}

@Test
public void testGeometryTypeWithCustomCrsConvertToHiveSchema() {

@gaborkaszab gaborkaszab Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test any different than the one above? It has a custom CRS, but doesn't seem to bring any extra test coverage.

@csringhofer csringhofer Jul 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Schema schema = new Schema(optional(0, "geometry_field", Types.GeometryType.of("EPSG:3857")));
List<FieldSchema> hiveSchema = HiveSchemaUtil.convert(schema);

@gaborkaszab gaborkaszab Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@csringhofer csringhofer Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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() {

@gaborkaszab gaborkaszab Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test below with both CRS and algorithm is enough.

@csringhofer csringhofer Jul 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will deal with this test once we resolve discussion on testGeometryTypeWithCustomCrsConvertToHiveSchema

Schema schema = new Schema(optional(0, "geography_field", Types.GeographyType.of("EPSG:4326")));
List<FieldSchema> hiveSchema = HiveSchemaUtil.convert(schema);
assertThat(hiveSchema).containsExactly(new FieldSchema("geography_field", "binary", null));
}

@Test
public void testGeographyTypeWithVincentyAlgorithmConvertToHiveSchema() {
Schema schema =
new Schema(
optional(
0, "geography_field", Types.GeographyType.of("OGC:CRS84", EdgeAlgorithm.VINCENTY)));
List<FieldSchema> hiveSchema = HiveSchemaUtil.convert(schema);
assertThat(hiveSchema).containsExactly(new FieldSchema("geography_field", "binary", null));
}

protected List<FieldSchema> getSupportedFieldSchemas() {
List<FieldSchema> fields = Lists.newArrayListWithCapacity(10);
fields.add(new FieldSchema("c_float", serdeConstants.FLOAT_TYPE_NAME, "float comment"));
Expand Down
Loading