wirybeaver commented on code in PR #2933:
URL: https://github.com/apache/iceberg-rust/pull/2933#discussion_r3942055829
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -100,6 +102,57 @@ impl ExtensionType for VariantExtensionType {
}
}
+fn edge_interpolation_algorithm_to_wkb_edges(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,
+ }
+}
+
+fn wkb_edges_to_edge_interpolation_algorithm(edges: WkbEdges) ->
EdgeInterpolationAlgorithm {
+ match edges {
+ WkbEdges::Spherical => EdgeInterpolationAlgorithm::Spherical,
+ WkbEdges::Vincenty => EdgeInterpolationAlgorithm::Vincenty,
+ WkbEdges::Thomas => EdgeInterpolationAlgorithm::Thomas,
+ WkbEdges::Andoyer => EdgeInterpolationAlgorithm::Andoyer,
+ WkbEdges::Karney => EdgeInterpolationAlgorithm::Karney,
+ }
+}
+
+fn iceberg_crs_from_wkb_metadata(crs: Option<&serde_json::Value>) ->
Result<Option<String>> {
+ match crs {
+ None => Ok(None),
Review Comment:
Fixed in dd55c899. A missing WKB CRS now maps to Iceberg `srid:0` instead of
the Iceberg default CRS.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -100,6 +102,57 @@ impl ExtensionType for VariantExtensionType {
}
}
+fn edge_interpolation_algorithm_to_wkb_edges(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,
+ }
+}
+
+fn wkb_edges_to_edge_interpolation_algorithm(edges: WkbEdges) ->
EdgeInterpolationAlgorithm {
+ match edges {
+ WkbEdges::Spherical => EdgeInterpolationAlgorithm::Spherical,
+ WkbEdges::Vincenty => EdgeInterpolationAlgorithm::Vincenty,
+ WkbEdges::Thomas => EdgeInterpolationAlgorithm::Thomas,
+ WkbEdges::Andoyer => EdgeInterpolationAlgorithm::Andoyer,
+ WkbEdges::Karney => EdgeInterpolationAlgorithm::Karney,
+ }
+}
+
+fn iceberg_crs_from_wkb_metadata(crs: Option<&serde_json::Value>) ->
Result<Option<String>> {
+ match crs {
+ None => Ok(None),
+ Some(serde_json::Value::String(crs)) => Ok(Some(crs.clone())),
Review Comment:
Addressed in dd55c899 by restoring the 128-byte CRS sanity limit. This keeps
accidentally escaped WKT2/PROJJSON strings out of Iceberg schema metadata
without adding broader CRS parsing.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -2506,6 +2516,96 @@ mod tests {
assert_eq!(std::fs::read_dir(temp_dir.path()).unwrap().count(), 0);
}
+ #[tokio::test]
+ async fn test_parquet_writer_geospatial_logical_types() -> Result<()> {
+ let temp_dir = TempDir::new().unwrap();
+ let file_io = FileIO::new_with_fs();
+ let location_gen = DefaultLocationGenerator::with_data_location(
+ temp_dir.path().to_str().unwrap().to_string(),
+ );
+ let file_name_gen =
+ DefaultFileNameGenerator::new("test".to_string(), None,
DataFileFormat::Parquet);
+
+ let schema = Arc::new(
+ Schema::builder()
+ .with_schema_id(1)
+ .with_fields(vec![
+ NestedField::required(
+ 0,
+ "geom",
+
Type::Primitive(PrimitiveType::Geometry(GeometryType::default())),
+ )
+ .into(),
+ NestedField::optional(
+ 1,
+ "geog",
+ Type::Primitive(PrimitiveType::Geography(
+ GeographyType::new(None,
IcebergEdgeInterpolationAlgorithm::Karney)
+ .unwrap(),
+ )),
+ )
+ .into(),
+ ])
+ .build()
+ .unwrap(),
+ );
+ let arrow_schema: ArrowSchemaRef =
Arc::new(schema_to_arrow_schema(&schema).unwrap());
+ let geom_wkb = wkb_point_xy(1.0, 2.0);
+ let geog_wkb = wkb_point_xy(3.0, 4.0);
+ let geom = Arc::new(arrow_array::LargeBinaryArray::from_vec(vec![
+ geom_wkb.as_slice(),
+ ])) as ArrayRef;
+ let geog = Arc::new(arrow_array::LargeBinaryArray::from_vec(vec![
+ geog_wkb.as_slice(),
+ ])) as ArrayRef;
+ let to_write = RecordBatch::try_new(arrow_schema.clone(), vec![geom,
geog]).unwrap();
+
+ let output_file = file_io.new_output(
+ location_gen.generate_location(None,
&file_name_gen.generate_file_name()),
+ )?;
+ let mut pw =
ParquetWriterBuilder::new(WriterProperties::builder().build(), schema)
+ .build(output_file)
+ .await?;
+
+ pw.write(&to_write).await?;
+ let res = pw.close().await?;
+ assert_eq!(res.len(), 1);
+ let data_file = res
+ .into_iter()
+ .next()
+ .unwrap()
+ .content(DataContentType::Data)
+ .partition(Struct::empty())
+ .partition_spec_id(0)
+ .build()
+ .unwrap();
+
+ assert_eq!(data_file.record_count(), 1);
+ assert!(data_file.lower_bounds().is_empty());
+ assert!(data_file.upper_bounds().is_empty());
+
+ let input_file = file_io.new_input(data_file.file_path())?;
+ let file_metadata = input_file.metadata().await?;
+ let reader = input_file.reader().await?;
+ let mut parquet_reader = ArrowFileReader::new(file_metadata, reader);
+ let parquet_metadata = parquet_reader.get_metadata(None).await?;
+ let schema_descr = parquet_metadata.file_metadata().schema_descr();
+
+ assert_eq!(
+ schema_descr.column(0).logical_type_ref(),
+ Some(&LogicalType::geometry(Some("srid:0".to_string())))
+ );
+ assert_eq!(
+ schema_descr.column(1).logical_type_ref(),
+ Some(&LogicalType::geography(
+ Some("srid:0".to_string()),
+ Some(EdgeInterpolationAlgorithm::KARNEY),
+ ))
+ );
Review Comment:
Fixed in dd55c899. The end-to-end writer test now checks default CRS ->
omitted Parquet CRS and explicit `srid:0` -> Parquet `srid:0`, while retaining
the Geography/Karney assertion.
##########
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);
+ }
+ Type::Primitive(PrimitiveType::Geometry(geometry)) => {
+ let metadata = WkbMetadata::new(geometry.crs(), None);
+
arrow_field.try_with_extension_type(WkbType::new(Some(metadata)))?;
+ }
+ Type::Primitive(PrimitiveType::Geography(geography)) => {
+ let metadata = WkbMetadata::new(
+ geography.crs(),
+ Some(edge_interpolation_algorithm_to_wkb_edges(
+ geography.algorithm(),
+ )),
+ );
Review Comment:
Fixed in dd55c899. Iceberg default CRS is now emitted to GeoArrow explicitly
as `OGC:CRS84`, while a missing GeoArrow CRS imports as `srid:0`. The Arrow
round-trip test covers the default-CRS path.
--
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]
