wirybeaver commented on code in PR #2933:
URL: https://github.com/apache/iceberg-rust/pull/2933#discussion_r3997433591
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -408,6 +590,25 @@ impl fmt::Display for PrimitiveType {
PrimitiveType::Uuid => write!(f, "uuid"),
PrimitiveType::Fixed(size) => write!(f, "fixed({size})"),
PrimitiveType::Binary => write!(f, "binary"),
+ PrimitiveType::Geometry(geometry) => match geometry.crs() {
+ Some(crs) => write!(f, "geometry({crs})"),
+ None => write!(f, "geometry"),
+ },
Review Comment:
Done in 07727701. Parsing remains tolerant of omitted defaults, while
display/JSON serialization now emits the canonical forms `geometry(OGC:CRS84)`
and `geography(OGC:CRS84, spherical)`.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -628,15 +717,31 @@ impl SchemaVisitor for ToArrowSchemaConverter {
} else {
HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(),
field.id.to_string())])
};
- let arrow_field =
+ let mut arrow_field =
Field::new(field.name.clone(), ty,
!field.required).with_metadata(metadata);
- // A variant column's storage is a struct; tag the field with the
canonical
- // `arrow.parquet.variant` extension type so consumers read it as a
Variant, not a struct.
- let arrow_field = if field.field_type.is_variant() {
- arrow_field.with_extension_type(VariantExtensionType)
- } else {
- arrow_field
- };
+
+ match field.field_type.as_ref() {
+ Type::Variant(_) => {
+ // A variant column's storage is a struct; tag the field with
the canonical
+ // `arrow.parquet.variant` extension type so consumers read it
as a Variant, not a struct.
+ arrow_field =
arrow_field.with_extension_type(VariantExtensionType);
+ }
Review Comment:
Done in 07727701. Variant now uses `try_with_extension_type`, consistent
with the geospatial extensions and without an infallible panic path.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -396,22 +449,55 @@ 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 =
iceberg_crs_from_wkb_metadata(wkb_type.metadata().crs.as_ref())?;
+
+ match wkb_type.metadata().type_hint() {
+ WkbTypeHint::Geometry =>
Ok(Type::Primitive(PrimitiveType::Geometry(
+ GeometryType::new(crs)?,
+ ))),
+ WkbTypeHint::Geography =>
Ok(Type::Primitive(PrimitiveType::Geography(
+ GeographyType::new(
+ crs,
+ wkb_edges_to_edge_interpolation_algorithm(
+ wkb_type.metadata().algorithm.unwrap_or_default(),
+ ),
Review Comment:
Done in 07727701. The missing Parquet edge metadata now explicitly maps to
`WkbEdges::Spherical` before conversion.
##########
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:
Agreed that the generic Iceberg JSON value representation for
geometry/geography is WKT. The current call sites here are schema defaults,
where the spec requires null, while implementing general WKT-to-WKB literal
conversion introduces a separate codec concern. I am leaving this thread open
rather than expanding this type-integration PR.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -396,22 +449,55 @@ 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])?;
Review Comment:
Variant must be recognized before descending into its struct storage because
its storage children intentionally have no Iceberg field IDs. WKB uses a
primitive binary storage type, so its extension can be applied after normal
type traversal. In the Iceberg-to-Arrow direction both extensions are attached
in the common field callback.
--
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]
