wirybeaver commented on code in PR #2933:
URL: https://github.com/apache/iceberg-rust/pull/2933#discussion_r3700709013
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -396,22 +418,71 @@ impl ArrowSchemaConverter {
let mut results = Vec::with_capacity(fields.len());
for i in 0..fields.len() {
let field = &fields[i];
- let field_type = &field_results[i];
+ let field_type = self.apply_field_extension_type(field,
&field_results[i])?;
let id = self.get_field_id(field)?;
let doc = get_field_doc(field);
let nested_field = NestedField {
id,
doc,
name: field.name().clone(),
required: !field.is_nullable(),
- field_type: Box::new(field_type.clone()),
+ field_type: Box::new(field_type),
initial_default: None,
write_default: None,
};
results.push(Arc::new(nested_field));
}
Ok(results)
}
+
+ fn apply_field_extension_type(&self, field: &FieldRef, field_type: &Type)
-> Result<Type> {
+ if field.extension_type_name() != Some(WkbType::NAME) {
+ return Ok(field_type.clone());
+ }
+
+ let wkb_type = field.try_extension_type::<WkbType>().map_err(|err| {
Review Comment:
Thanks for flagging this. I am leaving these corner cases as a limitation of
the current arrow-rs dependency for now; they will be resolved when
iceberg-rust upgrades to arrow-rs 59.1.0 or later.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -396,22 +418,71 @@ impl ArrowSchemaConverter {
let mut results = Vec::with_capacity(fields.len());
for i in 0..fields.len() {
let field = &fields[i];
- let field_type = &field_results[i];
+ let field_type = self.apply_field_extension_type(field,
&field_results[i])?;
let id = self.get_field_id(field)?;
let doc = get_field_doc(field);
let nested_field = NestedField {
id,
doc,
name: field.name().clone(),
required: !field.is_nullable(),
- field_type: Box::new(field_type.clone()),
+ field_type: Box::new(field_type),
initial_default: None,
write_default: None,
};
results.push(Arc::new(nested_field));
}
Ok(results)
}
+
+ fn apply_field_extension_type(&self, field: &FieldRef, field_type: &Type)
-> Result<Type> {
+ if field.extension_type_name() != Some(WkbType::NAME) {
+ return Ok(field_type.clone());
+ }
+
+ let wkb_type = field.try_extension_type::<WkbType>().map_err(|err| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Invalid geospatial Arrow extension metadata for field {}",
+ field.name()
+ ),
+ )
+ .with_source(err)
+ })?;
+
+ let crs = wkb_type.metadata().crs.as_ref().map(|crs| match crs {
+ serde_json::Value::String(value) => value.clone(),
+ other => other.to_string(),
+ });
Review Comment:
Addressed the CRS representation in 889ccfb: Arrow string CRS values are
retained, while PROJJSON is converted from its `id.authority` and `id.code` to
Iceberg-preferred `AUTHORITY:CODE`, with invalid metadata rejected. I kept
arrow-rs `WkbType` rather than introducing an Iceberg-specific copy because
parquet 58.4 consumes its own `WkbType`; replacing only the Iceberg Arrow type
would not fix the pre-59.1 Geography behavior and could make Parquet
logical-type writing incompatible.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -231,6 +232,171 @@ impl From<MapType> for Type {
}
}
+/// Iceberg geometry type.
+#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Hash, Default)]
+pub struct GeometryType {
+ crs: Option<String>,
+}
+
+impl GeometryType {
+ /// Creates a geometry type with an optional coordinate reference system.
+ pub fn new(crs: Option<String>) -> Result<Self> {
+ Ok(Self {
+ crs: normalize_crs(crs)?,
+ })
+ }
+
+ /// Returns the coordinate reference system, or `None` for the Iceberg
default CRS.
+ pub fn crs(&self) -> Option<&str> {
+ self.crs.as_deref()
+ }
+}
+
+/// Iceberg geography edge interpolation algorithm.
+#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Copy, Hash,
Default)]
+#[serde(rename_all = "lowercase")]
+pub enum EdgeInterpolationAlgorithm {
+ /// Spherical edge interpolation.
+ #[default]
+ Spherical,
+ /// Vincenty edge interpolation.
+ Vincenty,
+ /// Thomas edge interpolation.
+ Thomas,
+ /// Andoyer edge interpolation.
+ Andoyer,
+ /// Karney edge interpolation.
+ Karney,
+}
+
+/// Iceberg geography type.
+#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Hash)]
+pub struct GeographyType {
+ crs: Option<String>,
+ algorithm: EdgeInterpolationAlgorithm,
+}
+
+impl Default for GeographyType {
+ fn default() -> Self {
+ Self {
+ crs: None,
+ algorithm: EdgeInterpolationAlgorithm::Spherical,
+ }
+ }
+}
+
+impl GeographyType {
+ /// Creates a geography type with an optional coordinate reference system
and edge interpolation algorithm.
+ pub fn new(crs: Option<String>, algorithm: EdgeInterpolationAlgorithm) ->
Result<Self> {
+ Ok(Self {
+ crs: normalize_crs(crs)?,
+ algorithm,
+ })
+ }
+
+ /// Returns the coordinate reference system, or `None` for the Iceberg
default CRS.
+ pub fn crs(&self) -> Option<&str> {
+ self.crs.as_deref()
+ }
+
+ /// Returns the edge interpolation algorithm.
+ pub fn algorithm(&self) -> EdgeInterpolationAlgorithm {
+ self.algorithm
+ }
+}
+
+fn normalize_crs(crs: Option<String>) -> Result<Option<String>> {
+ let Some(crs) = crs else {
+ return Ok(None);
+ };
+ let crs = crs.trim().to_string();
+ if crs.is_empty() {
+ return Err(crate::Error::new(
+ crate::ErrorKind::DataInvalid,
+ "Geospatial CRS must not be empty",
+ ));
+ }
+ Ok((crs != DEFAULT_GEOSPATIAL_CRS).then_some(crs))
Review Comment:
Fixed in 889ccfb. CRS normalization now treats both `OGC:CRS84` and
`EPSG:4326` as the default, with tests for Geometry and Geography.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -2211,6 +2324,103 @@ mod tests {
);
}
+ #[test]
+ fn test_geospatial_arrow_schema_roundtrip() {
+ let schema = Schema::builder()
+ .with_schema_id(1)
+ .with_fields(vec![
+ NestedField::required(
+ 1,
+ "geom",
+ Type::Primitive(PrimitiveType::Geometry(
+
GeometryType::new(Some("srid:4326".to_string())).unwrap(),
+ )),
+ )
+ .into(),
+ NestedField::optional(
+ 2,
+ "geog",
+ Type::Primitive(PrimitiveType::Geography(
+ GeographyType::new(
+ Some("srid:3857".to_string()),
+ IcebergEdgeInterpolationAlgorithm::Karney,
+ )
+ .unwrap(),
+ )),
+ )
+ .into(),
+ NestedField::optional(
+ 3,
+ "geom_list",
+ Type::List(ListType::new(
+ NestedField::list_element(
+ 4,
+
Type::Primitive(PrimitiveType::Geometry(GeometryType::default())),
+ true,
+ )
+ .into(),
+ )),
+ )
+ .into(),
+ ])
+ .build()
+ .unwrap();
+
+ let arrow_schema = schema_to_arrow_schema(&schema).unwrap();
+ let geom = arrow_schema.field(0);
+ assert_eq!(geom.data_type(), &DataType::LargeBinary);
+ let geom_wkb = geom.try_extension_type::<WkbType>().unwrap();
+ assert_eq!(
+ geom_wkb
+ .metadata()
+ .crs
+ .as_ref()
+ .and_then(|crs| crs.as_str()),
+ Some("srid:4326")
+ );
+ assert!(matches!(
+ geom_wkb.metadata().type_hint(),
+ WkbTypeHint::Geometry
+ ));
+
+ let geog = arrow_schema.field(1);
+ assert_eq!(geog.data_type(), &DataType::LargeBinary);
+ let geog_wkb = geog.try_extension_type::<WkbType>().unwrap();
+ assert_eq!(
+ geog_wkb
+ .metadata()
+ .crs
+ .as_ref()
+ .and_then(|crs| crs.as_str()),
+ Some("srid:3857")
+ );
+ assert_eq!(geog_wkb.metadata().algorithm, Some(WkbEdges::Karney));
+ assert!(matches!(
+ geog_wkb.metadata().type_hint(),
+ WkbTypeHint::Geography
+ ));
+
+ let list = arrow_schema.field(2);
+ let DataType::List(element) = list.data_type() else {
+ panic!("Expected list field");
+ };
+ assert_eq!(element.data_type(), &DataType::LargeBinary);
+ assert!(
+ matches!(
+ element
+ .try_extension_type::<WkbType>()
+ .unwrap()
+ .metadata()
+ .type_hint(),
+ WkbTypeHint::Geometry
+ ),
+ "Expected list element to retain WKB extension metadata"
+ );
+
+ let converted = arrow_schema_to_schema(&arrow_schema).unwrap();
+ assert_eq!(converted.as_struct().fields(),
schema.as_struct().fields());
+ }
+
Review Comment:
Added explicit default `Geometry` and `Geography` Arrow round-trip coverage
in 889ccfb.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -2211,6 +2324,103 @@ mod tests {
);
}
+ #[test]
+ fn test_geospatial_arrow_schema_roundtrip() {
+ let schema = Schema::builder()
+ .with_schema_id(1)
+ .with_fields(vec![
+ NestedField::required(
+ 1,
+ "geom",
+ Type::Primitive(PrimitiveType::Geometry(
+
GeometryType::new(Some("srid:4326".to_string())).unwrap(),
+ )),
Review Comment:
Fixed in 889ccfb. The non-default Geometry test now uses `EPSG:3857`.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -2211,6 +2324,103 @@ mod tests {
);
}
+ #[test]
+ fn test_geospatial_arrow_schema_roundtrip() {
+ let schema = Schema::builder()
+ .with_schema_id(1)
+ .with_fields(vec![
+ NestedField::required(
+ 1,
+ "geom",
+ Type::Primitive(PrimitiveType::Geometry(
+
GeometryType::new(Some("srid:4326".to_string())).unwrap(),
+ )),
+ )
+ .into(),
+ NestedField::optional(
+ 2,
+ "geog",
+ Type::Primitive(PrimitiveType::Geography(
+ GeographyType::new(
+ Some("srid:3857".to_string()),
Review Comment:
Fixed in 889ccfb. The non-default Geography test now uses the valid
authority/code value `OGC:CRS27`.
--
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]
