From 58db3ccb04115be8fa0ffa4312bbdeef7ad66aec Mon Sep 17 00:00:00 2001 From: Kyle Barron Date: Fri, 31 Jan 2025 15:46:48 -0500 Subject: [PATCH] Fix stack overflow when converting `GeometryCollection` scalar to geo (#984) As described in a contained comment: We can't implement both ``` impl From> for geo::GeometryCollection ``` and ``` impl From> for geo::Geometry ``` because of this problematic blanket impl (https://github.com/georust/geo/blob/ef55eabe9029b27f753d4c40db9f656e3670202e/geo-types/src/geometry/geometry_collection.rs#L113-L120). Thus we need to choose either one or the other to implement. If we implemented only `for geo::Geometry`, then the default blanket impl for `geo::GeometryCollection` would be **wrong** because it would doubly-nest the `GeometryCollection`: ```rs GeometryCollection( [ GeometryCollection( GeometryCollection( [ Point( Point( Coord { x: 0.0, y: 0.0, }, ), ), ], ), ), ], ) ``` Therefore we must implement only `for geo::GeometryCollection` Closes #979 --- .../src/algorithm/geo/bounding_rect.rs | 28 ++++++ .../src/scalar/geometrycollection/scalar.rs | 85 +++++++++++++++++-- 2 files changed, 108 insertions(+), 5 deletions(-) diff --git a/rust/geoarrow/src/algorithm/geo/bounding_rect.rs b/rust/geoarrow/src/algorithm/geo/bounding_rect.rs index 1bcbac5c..45dc061e 100644 --- a/rust/geoarrow/src/algorithm/geo/bounding_rect.rs +++ b/rust/geoarrow/src/algorithm/geo/bounding_rect.rs @@ -142,3 +142,31 @@ impl BoundingRect for &dyn ChunkedNativeArray { } } } + +#[cfg(test)] +mod test { + use super::*; + + use geo::point; + + use crate::array::{GeometryCollectionArray, PointArray}; + use crate::datatypes::Dimension; + use crate::ArrayBase; + + #[test] + fn stack_overflow_repro_issue_979() { + let points: Vec> = vec![ + point! {x: -104.991, y: 39.7392}, + point! {x: -73.7897, y: 40.7379}, + ]; + let point_array: PointArray = (points.as_slice(), Dimension::XY).into(); + assert!(point_array.is_valid(0)); + assert!(point_array.is_valid(1)); + let _ = point_array.bounding_rect(); + + let gc_array: GeometryCollectionArray = point_array.into(); + assert!(gc_array.is_valid(0)); + assert!(gc_array.is_valid(1)); + let _ = gc_array.bounding_rect(); + } +} diff --git a/rust/geoarrow/src/scalar/geometrycollection/scalar.rs b/rust/geoarrow/src/scalar/geometrycollection/scalar.rs index fc3a9ad2..e52dcfb2 100644 --- a/rust/geoarrow/src/scalar/geometrycollection/scalar.rs +++ b/rust/geoarrow/src/scalar/geometrycollection/scalar.rs @@ -1,3 +1,4 @@ +use crate::algorithm::native::bounding_rect::bounding_rect_geometry_collection; use crate::algorithm::native::eq::geometry_collection_eq; use crate::array::util::OffsetBufferUtils; use crate::array::MixedGeometryArray; @@ -118,19 +119,69 @@ impl From<&GeometryCollection<'_>> for geo::GeometryCollection { } } -impl From> for geo::Geometry { +impl From> for geo::GeometryCollection { fn from(value: GeometryCollection<'_>) -> Self { - geo::Geometry::GeometryCollection(value.into()) + (&value).into() } } +// We can't implement both +// ``` +// impl From> for geo::GeometryCollection +// ``` +// and +// ``` +// impl From> for geo::Geometry +// ``` +// because of this problematic blanket impl +// (https://github.com/georust/geo/blob/ef55eabe9029b27f753d4c40db9f656e3670202e/geo-types/src/geometry/geometry_collection.rs#L113-L120). +// +// Thus we need to choose either one or the other to implement. +// +// If we implemented only `for geo::Geometry`, then the default blanket impl for +// `geo::GeometryCollection` would be **wrong** because it would doubly-nest the +// `GeometryCollection`: +// +// ```rs +// GeometryCollection( +// [ +// GeometryCollection( +// GeometryCollection( +// [ +// Point( +// Point( +// Coord { +// x: 0.0, +// y: 0.0, +// }, +// ), +// ), +// ], +// ), +// ), +// ], +// ) +// ``` +// +// Therefore we must implement only `for geo::GeometryCollection` +// impl From<&GeometryCollection<'_>> for geo::Geometry { +// fn from(value: &GeometryCollection<'_>) -> Self { +// geo::Geometry::GeometryCollection(geometry_collection_to_geo(value)) +// } +// } + +// impl From> for geo::Geometry { +// fn from(value: GeometryCollection<'_>) -> Self { +// geo::Geometry::GeometryCollection(geometry_collection_to_geo(&value)) +// } +// } + impl RTreeObject for GeometryCollection<'_> { type Envelope = AABB<[f64; 2]>; fn envelope(&self) -> Self::Envelope { - todo!() - // let (lower, upper) = bounding_rect_multilinestring(self); - // AABB::from_corners(lower, upper) + let (lower, upper) = bounding_rect_geometry_collection(self); + AABB::from_corners(lower, upper) } } @@ -139,3 +190,27 @@ impl> PartialEq for GeometryCollection<'_ geometry_collection_eq(self, other) } } + +#[cfg(test)] +mod tests { + use arrow_buffer::OffsetBufferBuilder; + + use crate::array::PointArray; + + use super::*; + + #[test] + fn stack_overflow_repro_issue_979() { + let orig_point = geo::point!(x: 0., y: 0.); + let array: MixedGeometryArray = + PointArray::from((vec![orig_point].as_slice(), Dimension::XY)).into(); + let mut offsets = OffsetBufferBuilder::new(1); + offsets.push_length(1); + let offsets = offsets.finish(); + let gc = GeometryCollection::new(&array, &offsets, 0); + + let out: geo::GeometryCollection = gc.into(); + assert_eq!(out.0.len(), 1, "should be one point"); + assert_eq!(out.0[0], geo::Geometry::Point(orig_point)); + } +}