wirybeaver commented on code in PR #2933:
URL: https://github.com/apache/iceberg-rust/pull/2933#discussion_r3997432918
##########
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"),
+ },
+ PrimitiveType::Geography(geography) => {
+ let algorithm = geography.algorithm();
+ match (geography.crs(), algorithm) {
+ (None, EdgeInterpolationAlgorithm::Spherical) => write!(f,
"geography"),
+ (Some(crs), EdgeInterpolationAlgorithm::Spherical) => {
+ write!(f, "geography({crs})")
+ }
+ (crs, algorithm) => write!(
+ f,
+ "geography({}, {})",
+ crs.unwrap_or(DEFAULT_GEOSPATIAL_CRS),
+ edge_interpolation_algorithm_as_str(algorithm)
+ ),
+ }
+ }
Review Comment:
Done in 07727701. Geography display now always includes both the canonical
CRS and edge algorithm, matching iceberg-java, including `spherical`.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -1042,6 +1243,56 @@ mod tests {
)
}
+ #[test]
+ fn primitive_type_geospatial() {
+ let cases = vec![
+ (
+ r#""geometry""#,
+ PrimitiveType::Geometry(GeometryType::default()),
+ "geometry",
+ ),
+ (
+ r#""geometry ( EPSG:3857 )""#,
+
PrimitiveType::Geometry(GeometryType::new(Some("EPSG:3857".to_string())).unwrap()),
+ "geometry(EPSG:3857)",
+ ),
+ (
+ r#""geography""#,
+ PrimitiveType::Geography(GeographyType::default()),
+ "geography",
+ ),
+ (
+ r#""geography ( OGC:CRS27 , karney )""#,
+ PrimitiveType::Geography(
+ GeographyType::new(
+ Some("OGC:CRS27".to_string()),
+ EdgeInterpolationAlgorithm::Karney,
+ )
+ .unwrap(),
+ ),
+ "geography(OGC:CRS27, karney)",
+ ),
+ ];
+
+ for (json, expected, display) in cases {
+ let actual: PrimitiveType = serde_json::from_str(json).unwrap();
+ assert_eq!(actual, expected);
+ assert_eq!(actual.to_string(), display);
+ assert_eq!(
+ serde_json::to_string(&actual).unwrap(),
+ format!(r#""{display}""#)
+ );
Review Comment:
Done in 07727701. Added the requested assertion messages.
##########
crates/iceberg/public-api.txt:
##########
@@ -1463,6 +1463,29 @@ impl serde_core::ser::Serialize for
iceberg::spec::DataFileFormat where Self: co
pub fn iceberg::spec::DataFileFormat::serialize<__S>(&self, serializer: __S)
-> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as
serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer
impl<'de> serde_core::de::Deserialize<'de> for iceberg::spec::DataFileFormat
where Self: core::str::traits::FromStr, <Self as
core::str::traits::FromStr>::Err: core::fmt::Display
pub fn iceberg::spec::DataFileFormat::deserialize<__D>(deserializer: __D) ->
core::result::Result<Self, <__D as serde_core::de::Deserializer>::Error> where
__D: serde_core::de::Deserializer<'de>
+pub enum iceberg::spec::EdgeInterpolationAlgorithm
+pub iceberg::spec::EdgeInterpolationAlgorithm::Andoyer
+pub iceberg::spec::EdgeInterpolationAlgorithm::Karney
+pub iceberg::spec::EdgeInterpolationAlgorithm::Spherical
+pub iceberg::spec::EdgeInterpolationAlgorithm::Thomas
+pub iceberg::spec::EdgeInterpolationAlgorithm::Vincenty
+impl core::clone::Clone for iceberg::spec::EdgeInterpolationAlgorithm
+pub fn iceberg::spec::EdgeInterpolationAlgorithm::clone(&self) ->
iceberg::spec::EdgeInterpolationAlgorithm
+impl core::cmp::Eq for iceberg::spec::EdgeInterpolationAlgorithm
+impl core::cmp::PartialEq for iceberg::spec::EdgeInterpolationAlgorithm
+pub fn iceberg::spec::EdgeInterpolationAlgorithm::eq(&self, other:
&iceberg::spec::EdgeInterpolationAlgorithm) -> bool
+impl core::default::Default for iceberg::spec::EdgeInterpolationAlgorithm
+pub fn iceberg::spec::EdgeInterpolationAlgorithm::default() ->
iceberg::spec::EdgeInterpolationAlgorithm
+impl core::fmt::Debug for iceberg::spec::EdgeInterpolationAlgorithm
+pub fn iceberg::spec::EdgeInterpolationAlgorithm::fmt(&self, f: &mut
core::fmt::Formatter<'_>) -> core::fmt::Result
+impl core::hash::Hash for iceberg::spec::EdgeInterpolationAlgorithm
+pub fn iceberg::spec::EdgeInterpolationAlgorithm::hash<__H:
core::hash::Hasher>(&self, state: &mut __H)
+impl core::marker::Copy for iceberg::spec::EdgeInterpolationAlgorithm
+impl core::marker::StructuralPartialEq for
iceberg::spec::EdgeInterpolationAlgorithm
+impl serde_core::ser::Serialize for iceberg::spec::EdgeInterpolationAlgorithm
+pub fn iceberg::spec::EdgeInterpolationAlgorithm::serialize<__S>(&self,
__serializer: __S) -> core::result::Result<<__S as
serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error>
where __S: serde_core::ser::Serializer
+impl<'de> serde_core::de::Deserialize<'de> for
iceberg::spec::EdgeInterpolationAlgorithm
+pub fn
iceberg::spec::EdgeInterpolationAlgorithm::deserialize<__D>(__deserializer:
__D) -> core::result::Result<Self, <__D as
serde_core::de::Deserializer>::Error> where __D:
serde_core::de::Deserializer<'de>
Review Comment:
I kept these in the existing flat `spec` namespace because they are
constituents of the public primitive type model, consistent with the other spec
datatypes. Moving the geospatial API into a new module is a broader public API
organization decision; I will leave this thread open for maintainer consensus.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -324,6 +499,10 @@ impl<'de> Deserialize<'de> for PrimitiveType {
deserialize_decimal(s.into_deserializer())
} else if s.starts_with("fixed") {
deserialize_fixed(s.into_deserializer())
+ } else if s.starts_with("geometry") {
+ parse_geometry(&s).map_err(D::Error::custom)
+ } else if s.starts_with("geography") {
+ parse_geography(&s).map_err(D::Error::custom)
Review Comment:
The difference is intentional: fixed binary and decimal are represented
directly by Arrow `DataType`, while GeoArrow WKB identity and CRS/edge metadata
live on the enclosing `Field` as extension metadata. The field conversion
applies that metadata after the storage type visitor returns.
--
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]
