CTTY commented on code in PR #2933:
URL: https://github.com/apache/iceberg-rust/pull/2933#discussion_r4201747222
##########
crates/catalog/glue/src/schema.rs:
##########
@@ -560,4 +568,23 @@ mod tests {
assert_eq!(result, expected);
Ok(())
}
+
+ #[test]
+ fn test_schema_with_geospatial_type_is_unsupported() {
Review Comment:
Accordingly, this test needs to be removed
##########
crates/catalog/glue/src/schema.rs:
##########
@@ -173,6 +173,12 @@ impl SchemaVisitor for GlueSchemaBuilder {
"string".to_string()
}
PrimitiveType::Binary | PrimitiveType::Fixed(_) =>
"binary".to_string(),
+ PrimitiveType::Geometry(_) | PrimitiveType::Geography(_) => {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ format!("Conversion from {p:?} is not supported"),
+ ));
+ }
Review Comment:
I think this and the existing glue schema conversion is wrong, and we should
not fail the type conversion here. Glue only need these columns for display for
Iceberg tables, and will always use metadata as the source of truth when
actually reading the data. We should just return `type.to_string()` for every
type
Since this is an existing problem, we don't have to fix it here. But could
you add a comment here and link an issue?
##########
crates/catalog/glue/src/schema.rs:
##########
@@ -173,6 +173,12 @@ impl SchemaVisitor for GlueSchemaBuilder {
"string".to_string()
}
PrimitiveType::Binary | PrimitiveType::Fixed(_) =>
"binary".to_string(),
+ PrimitiveType::Geometry(_) | PrimitiveType::Geography(_) => {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ format!("Conversion from {p:?} is not supported"),
+ ));
+ }
Review Comment:
Java is already doing what I mentioned above, and it looks correct:
https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/glue/IcebergToGlueConverter.java#L378
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -1178,6 +1182,7 @@ pub(crate) fn
primitive_type_to_arrow_type_with_ree(primitive_type: &PrimitiveTy
PrimitiveType::Uuid => make_ree(DataType::Binary),
PrimitiveType::Fixed(_) => make_ree(DataType::Binary),
PrimitiveType::Binary => make_ree(DataType::Binary),
+ PrimitiveType::Geometry(_) | PrimitiveType::Geography(_) =>
make_ree(DataType::LargeBinary),
Review Comment:
I'm not quite familiar with the actual Geospatial workload: can the binary
be over 2GB and we need to use `LargeBinary`?
--
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]
