wirybeaver commented on code in PR #2933:
URL: https://github.com/apache/iceberg-rust/pull/2933#discussion_r3722014675
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -2417,4 +2522,37 @@ mod tests {
assert_eq!(cdc.max_chunk_size, 8192);
assert_eq!(cdc.norm_level, 2);
}
+
+ #[test]
+ fn test_min_max_aggregator_skips_geospatial_byte_statistics() {
Review Comment:
Thanks. I will keep geospatial bounds/statistics out of scope for this PR;
the current writer explicitly avoids treating WKB byte ordering as geospatial
min/max statistics. Geography-aware bounds can be added in a focused follow-up.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -231,6 +233,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 && crs !=
EQUIVALENT_DEFAULT_GEOSPATIAL_CRS).then_some(crs))
+}
+
+fn edge_interpolation_algorithm_as_str(algorithm: EdgeInterpolationAlgorithm)
-> &'static str {
+ match algorithm {
+ EdgeInterpolationAlgorithm::Spherical => "spherical",
+ EdgeInterpolationAlgorithm::Vincenty => "vincenty",
+ EdgeInterpolationAlgorithm::Thomas => "thomas",
+ EdgeInterpolationAlgorithm::Andoyer => "andoyer",
+ EdgeInterpolationAlgorithm::Karney => "karney",
+ }
+}
+
+fn parse_edge_interpolation_algorithm(
+ value: &str,
+) -> std::result::Result<EdgeInterpolationAlgorithm, String> {
+ match value.trim().to_ascii_lowercase().as_str() {
+ "spherical" => Ok(EdgeInterpolationAlgorithm::Spherical),
+ "vincenty" => Ok(EdgeInterpolationAlgorithm::Vincenty),
+ "thomas" => Ok(EdgeInterpolationAlgorithm::Thomas),
+ "andoyer" => Ok(EdgeInterpolationAlgorithm::Andoyer),
+ "karney" => Ok(EdgeInterpolationAlgorithm::Karney),
+ _ => Err(format!(
+ "Unknown geography edge interpolation algorithm: {value}"
+ )),
+ }
+}
+
+fn parse_geospatial_params<'a>(
+ value: &'a str,
+ type_name: &str,
+) -> std::result::Result<Vec<&'a str>, String> {
+ if value == type_name {
+ return Ok(vec![]);
+ }
+
+ let params = value
+ .strip_prefix(&format!("{type_name}("))
Review Comment:
Yes. Appendix C says readers should accept optional whitespace around
parameters and separators. Updated the parser in 6c87c62 to accept forms such
as `geometry ( EPSG:3857 )` and `geography ( OGC:CRS27 , karney )`, while
continuing to serialize canonically.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -100,6 +102,73 @@ 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>> {
+ let Some(crs) = crs else {
+ return Ok(None);
+ };
+
+ match crs {
+ serde_json::Value::String(crs) => Ok(Some(crs.clone())),
Review Comment:
Added a 128-byte limit for CRS strings in 6c87c62, with boundary and Arrow
import tests. This prevents accidentally escaped WKT2/PROJJSON values from
adding large per-field schema overhead.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -100,6 +102,73 @@ 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>> {
+ let Some(crs) = crs else {
+ return Ok(None);
+ };
+
+ match crs {
+ serde_json::Value::String(crs) => Ok(Some(crs.clone())),
+ serde_json::Value::Object(_) => {
+ let id = crs
+ .get("id")
+ .and_then(serde_json::Value::as_object)
+ .ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "PROJJSON CRS must contain an id object",
+ )
Review Comment:
Updated in 6c87c62. A PROJJSON object without an embedded authority/code now
reports: `Cannot write PROJJSON CRS without an embedded authority/code to
Iceberg`.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -409,6 +589,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:
There is no escaping syntax in the Iceberg type-string grammar. To guarantee
that values produced by `Display` can be parsed back, 6c87c62 rejects CRS
strings containing `,` or `)` at construction time and adds round-trip
validation tests.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -100,6 +102,73 @@ 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>> {
+ let Some(crs) = crs else {
+ return Ok(None);
+ };
+
+ match crs {
+ serde_json::Value::String(crs) => Ok(Some(crs.clone())),
+ serde_json::Value::Object(_) => {
+ let id = crs
+ .get("id")
+ .and_then(serde_json::Value::as_object)
+ .ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "PROJJSON CRS must contain an id object",
+ )
+ })?;
+ let authority = id
+ .get("authority")
+ .and_then(serde_json::Value::as_str)
+ .filter(|authority| !authority.is_empty())
+ .ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "PROJJSON CRS id must contain a non-empty authority",
+ )
Review Comment:
Agreed. I kept these validation errors unchanged because they describe
malformed PROJJSON metadata rather than an Iceberg representation limitation.
--
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]
