paleolimbot commented on code in PR #2933:
URL: https://github.com/apache/iceberg-rust/pull/2933#discussion_r3704097583
##########
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:
You may want to check here that the crs is less than some threshold of bytes
(say, 128 bytes). Alternatively, you could validate that it looks like an
authority:code string but that is harder. Mostly you want to catch WKT2 CRSes
or PROJJSON CRSes that were accidentally escaped as strings to avoid manifest
files that are written with many kilobytes of overhead per geometry field.
##########
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:
Is there allowed to be a whitespace between the type name and the
parameters? (`GEOMETRY (` vs `GEOMETRY(`)?
##########
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:
A more informative error here might be "Can't write PROJJSON CRS without
embedded authority/code to iceberg".
##########
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:
I think these errors are OK (since they speak to invalid PROJJSON rather
than a limitation of the iceberg format).
##########
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:
Do you have to escape the CRS at all here? It is an arbitrary string and
could contain `)` or `,` which I don't think would roundtrip (and may be
invalid). Perhaps neither of those are concerns here.
##########
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:
No problem if statistics are out of scope for this PR (but I'm also happy to
review their addition here or in a follow up). Getting geography statistics
from the Parquet writer are a bit of a pain (you have to register a static
geography-aware bounder, an example of which we have in SedonaDB).
--
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]
