wirybeaver commented on code in PR #2933:
URL: https://github.com/apache/iceberg-rust/pull/2933#discussion_r4179090520
##########
crates/iceberg/src/spec/values/literal.rs:
##########
@@ -534,6 +534,13 @@ impl Literal {
(PrimitiveType::Binary, JsonValue::String(s)) =>
Ok(Some(Literal::Primitive(
PrimitiveLiteral::Binary(decode_hex_bytes(&s)?),
))),
+ (
+ PrimitiveType::Geometry(_) | PrimitiveType::Geography(_),
+ JsonValue::String(_),
+ ) => Err(Error::new(
+ ErrorKind::DataInvalid,
+ "Geometry and geography defaults must be null",
+ )),
Review Comment:
Addressed in fa95b817: geospatial JSON strings now return
`FeatureUnsupported` with an explicit WKT-not-yet-supported message. Non-null
schema defaults therefore remain safely rejected without claiming WKT itself is
invalid.
##########
crates/catalog/hms/src/schema.rs:
##########
@@ -117,7 +117,10 @@ impl SchemaVisitor for HiveSchemaBuilder {
PrimitiveType::Time | PrimitiveType::String | PrimitiveType::Uuid
=> {
"string".to_string()
}
- PrimitiveType::Binary | PrimitiveType::Fixed(_) =>
"binary".to_string(),
+ PrimitiveType::Binary
+ | PrimitiveType::Fixed(_)
+ | PrimitiveType::Geometry(_)
+ | PrimitiveType::Geography(_) => "binary".to_string(),
Review Comment:
Addressed in fa95b817: both HMS and Glue now return `FeatureUnsupported` for
geometry/geography instead of advertising them as generic binary. Both catalogs
have regression tests.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -100,6 +103,59 @@ impl ExtensionType for VariantExtensionType {
}
}
+fn wkb_edges_from_edge_interpolation_algorithm(algorithm:
EdgeInterpolationAlgorithm) -> WkbEdges {
+ match algorithm {
+ EdgeInterpolationAlgorithm::Spherical => WkbEdges::Spherical,
+ EdgeInterpolationAlgorithm::Vincenty => WkbEdges::Vincenty,
+ EdgeInterpolationAlgorithm::Thomas => WkbEdges::Thomas,
+ EdgeInterpolationAlgorithm::Andoyer => WkbEdges::Andoyer,
+ EdgeInterpolationAlgorithm::Karney => WkbEdges::Karney,
+ }
+}
Review Comment:
Addressed in the read-path commit 73105dba (#3341): renamed to
`to_arrow_wkb_edges` so the conversion direction is explicit.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -100,6 +103,59 @@ impl ExtensionType for VariantExtensionType {
}
}
+fn wkb_edges_from_edge_interpolation_algorithm(algorithm:
EdgeInterpolationAlgorithm) -> WkbEdges {
+ match algorithm {
+ EdgeInterpolationAlgorithm::Spherical => WkbEdges::Spherical,
+ EdgeInterpolationAlgorithm::Vincenty => WkbEdges::Vincenty,
+ EdgeInterpolationAlgorithm::Thomas => WkbEdges::Thomas,
+ EdgeInterpolationAlgorithm::Andoyer => WkbEdges::Andoyer,
+ EdgeInterpolationAlgorithm::Karney => WkbEdges::Karney,
+ }
+}
+
+impl From<WkbEdges> for EdgeInterpolationAlgorithm {
+ fn from(edges: WkbEdges) -> Self {
+ match edges {
+ WkbEdges::Spherical => Self::Spherical,
+ WkbEdges::Vincenty => Self::Vincenty,
+ WkbEdges::Thomas => Self::Thomas,
+ WkbEdges::Andoyer => Self::Andoyer,
+ WkbEdges::Karney => Self::Karney,
+ }
+ }
+}
+
+fn iceberg_crs_from_wkb_metadata(crs: Option<&serde_json::Value>) ->
Result<Option<String>> {
+ match crs {
+ None => Ok(Some(UNSET_GEOSPATIAL_CRS.to_string())),
+ Some(serde_json::Value::String(crs)) => Ok(Some(crs.clone())),
+ Some(serde_json::Value::Object(crs)) => {
+ let id = crs.get("id");
+ let authority = id
+ .and_then(|id| id.get("authority"))
+ .and_then(serde_json::Value::as_str)
+ .filter(|authority| !authority.is_empty());
+ let code = match id.and_then(|id| id.get("code")) {
+ Some(serde_json::Value::String(code)) => Some(code.clone()),
+ Some(serde_json::Value::Number(code)) =>
Some(code.to_string()),
+ _ => None,
+ };
+
+ match (authority, code) {
+ (Some(authority), Some(code)) =>
Ok(Some(format!("{authority}:{code}"))),
+ _ => Err(Error::new(
+ ErrorKind::DataInvalid,
+ "Cannot write PROJJSON CRS without an embedded
authority/code to Iceberg",
+ )),
+ }
+ }
+ Some(_) => Err(Error::new(
+ ErrorKind::DataInvalid,
+ "Geospatial CRS metadata must be a string or PROJJSON object",
+ )),
+ }
+}
Review Comment:
Addressed in 73105dba (#3341): CRS extraction now takes the complete
`WkbMetadata`, returns `Result<String>`, defaults missing CRS to `srid:0`, and
rejects malformed PROJJSON or non-string/object CRS values. Added invalid-CRS
tests.
--
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]
