[thrift-remodel] Incorporate changes made to geospatial statistics

parquet/src/basic.rs

@@ -349,15 +349,16 @@ pub enum LogicalType {
     },
     /// A geospatial feature in the Well-Known Binary (WKB) format with linear/planar edges interpolation.
     Geometry {
-        /// A custom CRS. If unset the defaults to `OGC:CRS84`.
+        /// A custom CRS. If unset the defaults to `OGC:CRS84`, which means that the geometries
+        /// must be stored in longitude, latitude based on the WGS84 datum.
         crs: Option<String>,
     },
     /// A geospatial feature in the WKB format with an explicit (non-linear/non-planar) edges interpolation.
     Geography {
         /// A custom CRS. If unset the defaults to `OGC:CRS84`.
         crs: Option<String>,
         /// An optional algorithm can be set to correctly interpret edges interpolation
-        /// of the geometries. If unset, the algorithm defaults to `SPHERICAL``.
+        /// of the geometries. If unset, the algorithm defaults to `SPHERICAL`.
         algorithm: Option<EdgeInterpolationAlgorithm>,
     },
     /// For forward compatibility; used when an unknown union value is encountered.
@@ -456,9 +457,10 @@ impl<'a, R: ThriftCompactInputProtocol<'a>> ReadThrift<'a, R> for LogicalType {
             }
             18 => {
                 let val = GeographyType::read_thrift(&mut *prot)?;
+                let algorithm = val.algorithm.unwrap_or_default();
                 Self::Geography {
                     crs: val.crs.map(|s| s.to_owned()),
-                    algorithm: val.algorithm,
+                    algorithm: Some(algorithm),
                 }
             }
             _ => {
@@ -928,17 +930,31 @@ enum BoundaryOrder {
 // ----------------------------------------------------------------------
 // Mirrors thrift enum `EdgeInterpolationAlgorithm`
 
+// TODO(ets): we need to allow for unknown variants. Either hand code this one, or add a new
+// macro that adds an _Unknown variant.
+
 thrift_enum!(
 /// Edge interpolation algorithm for Geography logical type
 enum EdgeInterpolationAlgorithm {
+  /// Edges are interpolated as geodesics on a sphere.
   SPHERICAL = 0;
+  /// <https://en.wikipedia.org/wiki/Vincenty%27s_formulae>
   VINCENTY = 1;
+  /// Thomas, Paul D. Spheroidal geodesics, reference systems, & local geometry. US Naval Oceanographic Office, 1970
   THOMAS = 2;
+  /// Thomas, Paul D. Mathematical models for navigation systems. US Naval Oceanographic Office, 1965.
   ANDOYER = 3;
+  /// Karney, Charles FF. "Algorithms for geodesics." Journal of Geodesy 87 (2013): 43-55
   KARNEY = 4;
 }
 );
 
+impl Default for EdgeInterpolationAlgorithm {
+    fn default() -> Self {
+        Self::SPHERICAL
+    }
+}
+
 // ----------------------------------------------------------------------
 // Mirrors thrift union `BloomFilterAlgorithm`
 
@@ -1359,7 +1375,7 @@ impl str::FromStr for LogicalType {
             "GEOMETRY" => Ok(LogicalType::Geometry { crs: None }),
             "GEOGRAPHY" => Ok(LogicalType::Geography {
                 crs: None,
-                algorithm: None,
+                algorithm: Some(EdgeInterpolationAlgorithm::SPHERICAL),
             }),
             other => Err(general_err!("Invalid parquet logical type {}", other)),
         }
@@ -1816,6 +1832,17 @@ mod tests {
             ConvertedType::from(Some(LogicalType::Float16)),
             ConvertedType::NONE
         );
+        assert_eq!(
+            ConvertedType::from(Some(LogicalType::Geometry { crs: None })),
+            ConvertedType::NONE
+        );
+        assert_eq!(
+            ConvertedType::from(Some(LogicalType::Geography {
+                crs: None,
+                algorithm: Some(EdgeInterpolationAlgorithm::default()),
+            })),
+            ConvertedType::NONE
+        );
         assert_eq!(
             ConvertedType::from(Some(LogicalType::Unknown)),
             ConvertedType::NONE
@@ -1897,11 +1924,11 @@ mod tests {
         });
         test_roundtrip(LogicalType::Geography {
             crs: Some("foo".to_owned()),
-            algorithm: None,
+            algorithm: Some(EdgeInterpolationAlgorithm::SPHERICAL),
         });
         test_roundtrip(LogicalType::Geography {
             crs: None,
-            algorithm: None,
+            algorithm: Some(EdgeInterpolationAlgorithm::SPHERICAL),
         });
     }
 
@@ -2113,7 +2140,15 @@ mod tests {
         check_sort_order(signed, SortOrder::SIGNED);
 
         // Undefined comparison
-        let undefined = vec![LogicalType::List, LogicalType::Map];
+        let undefined = vec![
+            LogicalType::List,
+            LogicalType::Map,
+            LogicalType::Geometry { crs: None },
+            LogicalType::Geography {
+                crs: None,
+                algorithm: Some(EdgeInterpolationAlgorithm::default()),
+            },
+        ];
         check_sort_order(undefined, SortOrder::UNDEFINED);
     }
 

parquet/src/file/metadata/thrift_gen.rs

@@ -1585,7 +1585,7 @@ impl WriteThrift for crate::geospatial::statistics::GeospatialStatistics {
 
     fn write_thrift<W: Write>(&self, writer: &mut ThriftCompactOutputProtocol<W>) -> Result<()> {
         let mut last_field_id = 0i16;
-        if let Some(bbox) = self.bbox() {
+        if let Some(bbox) = self.bounding_box() {
             last_field_id = bbox.write_thrift_field(writer, 1, last_field_id)?;
         }
         if let Some(geo_types) = self.geospatial_types() {

parquet/src/geospatial/statistics.rs

@@ -58,12 +58,12 @@ impl GeospatialStatistics {
         }
     }
 
-    /// Return the optional `BoundingBox`.
-    pub fn bbox(&self) -> Option<&BoundingBox> {
+    /// Optional bounding defining the spatial extent, where `None` represents a lack of information.
+    pub fn bounding_box(&self) -> Option<&BoundingBox> {
         self.bbox.as_ref()
     }
 
-    /// Return the optional list of geospatial types.
+    /// Optional list of geometry type identifiers, where `None` represents a lack of information.
     pub fn geospatial_types(&self) -> Option<&Vec<i32>> {
         self.geospatial_types.as_ref()
     }

parquet/src/schema/printer.rs

@@ -329,9 +329,20 @@ fn print_logical_and_converted(
             LogicalType::Variant {
                 specification_version,
             } => format!("VARIANT({specification_version:?})"),
-            LogicalType::Geometry { crs } => format!("GEOMETRY({crs:?})"),
+            LogicalType::Geometry { crs } => {
+                if let Some(crs) = crs {
+                    format!("GEOMETRY({crs})")
+                } else {
+                    "GEOMETRY".to_string()
+                }
+            }
             LogicalType::Geography { crs, algorithm } => {
-                format!("GEOGRAPHY({crs:?},{algorithm:?})")
+                let algorithm = algorithm.unwrap_or_default();
+                if let Some(crs) = crs {
+                    format!("GEOGRAPHY({algorithm}, {crs})")
+                } else {
+                    format!("GEOGRAPHY({algorithm})")
+                }
             }
             LogicalType::Unknown => "UNKNOWN".to_string(),
             LogicalType::_Unknown { field_id } => format!("_Unknown({field_id})"),
@@ -454,7 +465,7 @@ mod tests {
 
     use std::sync::Arc;
 
-    use crate::basic::{Repetition, Type as PhysicalType};
+    use crate::basic::{EdgeInterpolationAlgorithm, Repetition, Type as PhysicalType};
     use crate::errors::Result;
     use crate::schema::parser::parse_message_type;
 
@@ -784,6 +795,62 @@ mod tests {
                 .unwrap(),
                 "REQUIRED BYTE_ARRAY field [42] (STRING);",
             ),
+            (
+                build_primitive_type(
+                    "field",
+                    None,
+                    PhysicalType::BYTE_ARRAY,
+                    Some(LogicalType::Geometry { crs: None }),
+                    ConvertedType::NONE,
+                    Repetition::REQUIRED,
+                )
+                .unwrap(),
+                "REQUIRED BYTE_ARRAY field (GEOMETRY);",
+            ),
+            (
+                build_primitive_type(
+                    "field",
+                    None,
+                    PhysicalType::BYTE_ARRAY,
+                    Some(LogicalType::Geometry {
+                        crs: Some("non-missing CRS".to_string()),
+                    }),
+                    ConvertedType::NONE,
+                    Repetition::REQUIRED,
+                )
+                .unwrap(),
+                "REQUIRED BYTE_ARRAY field (GEOMETRY(non-missing CRS));",
+            ),
+            (
+                build_primitive_type(
+                    "field",
+                    None,
+                    PhysicalType::BYTE_ARRAY,
+                    Some(LogicalType::Geography {
+                        crs: None,
+                        algorithm: Some(EdgeInterpolationAlgorithm::default()),
+                    }),
+                    ConvertedType::NONE,
+                    Repetition::REQUIRED,
+                )
+                .unwrap(),
+                "REQUIRED BYTE_ARRAY field (GEOGRAPHY(SPHERICAL));",
+            ),
+            (
+                build_primitive_type(
+                    "field",
+                    None,
+                    PhysicalType::BYTE_ARRAY,
+                    Some(LogicalType::Geography {
+                        crs: Some("non-missing CRS".to_string()),
+                        algorithm: Some(EdgeInterpolationAlgorithm::default()),
+                    }),
+                    ConvertedType::NONE,
+                    Repetition::REQUIRED,
+                )
+                .unwrap(),
+                "REQUIRED BYTE_ARRAY field (GEOGRAPHY(SPHERICAL, non-missing CRS));",
+            ),
         ];
 
         types_and_strings.into_iter().for_each(|(field, expected)| {

parquet/tests/geospatial.rs

@@ -0,0 +1,123 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Tests for Geometry and Geography logical types
+use parquet::{
+    basic::{EdgeInterpolationAlgorithm, LogicalType},
+    file::{
+        metadata::ParquetMetaData,
+        reader::{FileReader, SerializedFileReader},
+    },
+    geospatial::bounding_box::BoundingBox,
+};
+use serde_json::Value;
+use std::fs::File;
+
+fn read_metadata(geospatial_test_file: &str) -> ParquetMetaData {
+    let path = format!(
+        "{}/geospatial/{geospatial_test_file}",
+        arrow::util::test_util::parquet_test_data(),
+    );
+    let file = File::open(path).unwrap();
+    let reader = SerializedFileReader::try_from(file).unwrap();
+    reader.metadata().clone()
+}
+
+#[test]
+fn test_read_logical_type() {
+    // Some crs values are short strings
+    let expected_logical_type = [
+        ("crs-default.parquet", LogicalType::Geometry { crs: None }),
+        (
+            "crs-srid.parquet",
+            LogicalType::Geometry {
+                crs: Some("srid:5070".to_string()),
+            },
+        ),
+        (
+            "crs-projjson.parquet",
+            LogicalType::Geometry {
+                crs: Some("projjson:projjson_epsg_5070".to_string()),
+            },
+        ),
+        (
+            "crs-geography.parquet",
+            LogicalType::Geography {
+                crs: None,
+                algorithm: Some(EdgeInterpolationAlgorithm::SPHERICAL),
+            },
+        ),
+    ];
+
+    for (geospatial_file, expected_type) in expected_logical_type {
+        let metadata = read_metadata(geospatial_file);
+        let logical_type = metadata
+            .file_metadata()
+            .schema_descr()
+            .column(1)
+            .logical_type()
+            .unwrap();
+
+        assert_eq!(logical_type, expected_type);
+    }
+
+    // The crs value may also contain arbitrary values (in this case some JSON
+    // a bit too lengthy to type out)
+    let metadata = read_metadata("crs-arbitrary-value.parquet");
+    let logical_type = metadata
+        .file_metadata()
+        .schema_descr()
+        .column(1)
+        .logical_type()
+        .unwrap();
+
+    if let LogicalType::Geometry { crs } = logical_type {
+        let crs_parsed: Value = serde_json::from_str(&crs.unwrap()).unwrap();
+        assert_eq!(crs_parsed.get("id").unwrap().get("code").unwrap(), 5070);
+    } else {
+        panic!("Expected geometry type but got {logical_type:?}");
+    }
+}
+
+#[test]
+fn test_read_geospatial_statistics() {
+    let metadata = read_metadata("geospatial.parquet");
+
+    // geospatial.parquet schema:
+    //    optional binary field_id=-1 group (String);
+    //    optional binary field_id=-1 wkt (String);
+    //    optional binary field_id=-1 geometry (Geometry(crs=));
+    let fields = metadata.file_metadata().schema().get_fields();
+    let logical_type = fields[2].get_basic_info().logical_type().unwrap();
+    assert_eq!(logical_type, LogicalType::Geometry { crs: None });
+
+    let geo_statistics = metadata.row_group(0).column(2).geo_statistics();
+    assert!(geo_statistics.is_some());
+
+    let expected_bbox = BoundingBox::new(10.0, 40.0, 10.0, 40.0)
+        .with_zrange(30.0, 80.0)
+        .with_mrange(200.0, 1600.0);
+    let expected_geospatial_types = vec![
+        1, 2, 3, 4, 5, 6, 7, 1001, 1002, 1003, 1004, 1005, 1006, 1007, 2001, 2002, 2003, 2004,
+        2005, 2006, 2007, 3001, 3002, 3003, 3004, 3005, 3006, 3007,
+    ];
+    assert_eq!(
+        geo_statistics.unwrap().geospatial_types(),
+        Some(&expected_geospatial_types)
+    );
+    assert_eq!(geo_statistics.unwrap().bounding_box(), Some(&expected_bbox));
+}