fix: Use topological (recursive) emptiness

rust/geodatafusion/src/udf/native/accessors/is_empty.rs

@@ -61,7 +61,10 @@ impl ScalarUDFImpl for IsEmpty {
         Some(DOCUMENTATION.get_or_init(|| {
             Documentation::builder(
                 DOC_SECTION_OTHER,
-                "Tests if a geometry is empty. ST_IsEmpty(NULL) is NULL.",
+                "Tests if a geometry is topologically empty. \
+                 Multi-geometries and GeometryCollections where every leaf is empty \
+                 (e.g. GEOMETRYCOLLECTION(POINT EMPTY, POLYGON EMPTY) are reported empty. \
+                 ST_IsEmpty(NULL) is NULL.",
                 "ST_IsEmpty(geom)",
             )
             .with_argument("geom", "geometry")
@@ -70,6 +73,38 @@ impl ScalarUDFImpl for IsEmpty {
     }
 }
 
+/// Recursive emptiness predicate.
+///
+/// This matches the behavior of PostGIS and JTS.
+/// As a cross-reference, the javadoc for JTS `Geometry.isEmpty()` reads as follows:
+/// "the emptiness test checks for topological emptiness,
+/// not structural emptiness. A collection containing only empty elements is reported as empty."
+pub(crate) fn is_geometry_topologically_empty(geom: &impl GeometryTrait<T = f64>) -> bool {
+    match geom.as_type() {
+        geo_traits::GeometryType::Point(p) => p.coord().is_none(),
+        geo_traits::GeometryType::LineString(ls) => ls.num_coords() == 0,
+        geo_traits::GeometryType::Polygon(p) => {
+            p.exterior().is_none_or(|ring| ring.num_coords() == 0)
+        }
+        geo_traits::GeometryType::MultiPoint(mp) => mp.points().all(|p| p.coord().is_none()),
+        geo_traits::GeometryType::MultiLineString(mls) => {
+            mls.line_strings().all(|ls| ls.num_coords() == 0)
+        }
+        geo_traits::GeometryType::MultiPolygon(mp) => mp
+            .polygons()
+            .all(|p| p.exterior().is_none_or(|ring| ring.num_coords() == 0)),
+        geo_traits::GeometryType::GeometryCollection(gc) => gc
+            .geometries()
+            .all(|child| is_geometry_topologically_empty(&child)),
+        // Rect/Triangle/Line always carry coordinates and so are never empty.
+        // NB: geoarrow has no curved-geometry support, so e.g. CIRCULARSTRING EMPTY
+        // from the PostGIS docs is not representable at the moment, but we can still model them.
+        geo_traits::GeometryType::Rect(_)
+        | geo_traits::GeometryType::Triangle(_)
+        | geo_traits::GeometryType::Line(_) => false,
+    }
+}
+
 fn is_empty_impl(args: ScalarFunctionArgs) -> GeoDataFusionResult<ColumnarValue> {
     let array = ColumnarValue::values_to_arrays(&args.args)?
         .into_iter()
@@ -92,28 +127,11 @@ fn impl_is_empty<'a>(
     for item in array.iter() {
         match item {
             // A present geometry.
-            // Emptiness is the absence of coordinates,
-            // which is distinct from a SQL NULL (handled below).
+            // Emptiness is recursive so that it's topological rather than just the structure.
+            // so a collection where every leaf is empty counts as empty itself.
             Some(geom) => {
                 let geom = geom?;
-                let is_empty = match geom.as_type() {
-                    geo_traits::GeometryType::Point(p) => p.coord().is_none(),
-                    geo_traits::GeometryType::LineString(ls) => ls.num_coords() == 0,
-                    geo_traits::GeometryType::Polygon(p) => {
-                        p.exterior().is_none_or(|ring| ring.num_coords() == 0)
-                    }
-                    geo_traits::GeometryType::MultiPoint(mp) => mp.num_points() == 0,
-                    geo_traits::GeometryType::MultiLineString(mls) => mls.num_line_strings() == 0,
-                    geo_traits::GeometryType::MultiPolygon(mp) => mp.num_polygons() == 0,
-                    geo_traits::GeometryType::GeometryCollection(gc) => gc.num_geometries() == 0,
-                    // Rect/Triangle/Line always carry coordinates and so are never empty.
-                    // (geoarrow has no curved-geometry support, so e.g. CIRCULARSTRING EMPTY
-                    // from the PostGIS docs is not representable here.)
-                    geo_traits::GeometryType::Rect(_)
-                    | geo_traits::GeometryType::Triangle(_)
-                    | geo_traits::GeometryType::Line(_) => false,
-                };
-                builder.append_value(is_empty);
+                builder.append_value(is_geometry_topologically_empty(&geom));
             }
             // SQL NULL in, SQL NULL out.
             // This matches the PostGIS behavior,
@@ -134,10 +152,10 @@ mod test {
     use datafusion::prelude::SessionContext;
 
     use super::*;
-    use crate::udf::native::io::GeomFromText;
+    use crate::udf::native::io::{GeomFromText, GeomFromWKB};
 
     #[tokio::test]
-    async fn test_st_isempty() {
+    async fn test_st_isempty_wkt() {
         let ctx = SessionContext::new();
         ctx.register_udf(IsEmpty::new().into());
         ctx.register_udf(GeomFromText::new(Default::default()).into());
@@ -203,6 +221,103 @@ mod test {
         }
     }
 
+    #[tokio::test]
+    async fn test_structural_emptiness() {
+        let ctx = SessionContext::new();
+        ctx.register_udf(IsEmpty::new().into());
+        ctx.register_udf(GeomFromText::new(Default::default()).into());
+        ctx.register_udf(GeomFromWKB::new(Default::default()).into());
+
+        // Recursive emptiness (PostGIS / JTS topological semantics).
+        // A collection whose every leaf is empty is itself reported as empty,
+        // even though it has structurally non-zero children.
+        let wkt_cases = vec![
+            // NB: GeoArrow explicitly denies recursive collections,
+            // so we only need to test one level.
+            (
+                "GEOMETRYCOLLECTION(POINT EMPTY, POLYGON EMPTY)",
+                true,
+                "collection of all-empty atomic children is recursively empty",
+            ),
+            (
+                "GEOMETRYCOLLECTION(MULTIPOINT EMPTY, MULTIPOLYGON EMPTY)",
+                true,
+                "collection of all-empty multi-children is recursively empty",
+            ),
+            (
+                "GEOMETRYCOLLECTION(POINT(0 0), POLYGON EMPTY)",
+                false,
+                "collection with at least one non-empty child is non-empty",
+            ),
+            (
+                "MULTILINESTRING(EMPTY, EMPTY)",
+                true,
+                "multi-geometry where every member is empty is recursively empty",
+            ),
+            (
+                "MULTILINESTRING(EMPTY, (0 0, 1 1))",
+                false,
+                "multi-geometry with at least one non-empty member is non-empty",
+            ),
+        ];
+
+        for (wkt, expected, description) in wkt_cases {
+            let sql = format!("SELECT ST_IsEmpty(ST_GeomFromText('{}'))", wkt);
+            let df = ctx
+                .sql(&sql)
+                .await
+                .unwrap_or_else(|_| panic!("Failed to execute SQL for {}", description));
+
+            let batch = df.collect().await.unwrap().into_iter().next().unwrap();
+            let col = batch.column(0).as_boolean();
+
+            assert_eq!(col.value(0), expected, "Failed on {}: {}", description, wkt);
+        }
+
+        // The following fixtures fail to parse or crash in WKT form, so we test via WKB
+        let wkb_cases = vec![
+            (
+                // MULTIPOINT (EMPTY, EMPTY)
+                "010400000000000000",
+                true,
+                "multi-geometry where every member is empty is recursively empty",
+            ),
+            (
+                // MULTIPOLYGON (EMPTY, EMPTY)
+                "010600000000000000",
+                true,
+                "multi-geometry where every member is empty is recursively empty",
+            ),
+            (
+                // MULTIPOINT(EMPTY, (0 0))
+                "0104000000020000000101000000000000000000f87f000000000000f87f010100000000000000000000000000000000000000",
+                false,
+                "multi-geometry with at least one non-empty member is non-empty",
+            ),
+            // This crashes geoarrow-array-0.8.0/src/builder/multipolygon.rs:188:51.
+            // There's an unconditional unwrap.
+            // (
+            //     // MULTIPOLYGON(EMPTY, ((0 0, 1 0, 1 1, 0 0)))
+            //     "0106000000020000000103000000000000000103000000010000000400000000000000000000000000000000000000000000000000f03f0000000000000000000000000000f03f000000000000f03f00000000000000000000000000000000",
+            //     false,
+            //     "multi-geometry with at least one non-empty member is non-empty",
+            // ),
+        ];
+
+        for (wkb, expected, description) in wkb_cases {
+            let sql = format!("SELECT ST_IsEmpty(ST_GeomFromWKB(X'{}'))", wkb);
+            let df = ctx
+                .sql(&sql)
+                .await
+                .unwrap_or_else(|_| panic!("Failed to execute SQL for {}", description));
+
+            let batch = df.collect().await.unwrap().into_iter().next().unwrap();
+            let col = batch.column(0).as_boolean();
+
+            assert_eq!(col.value(0), expected, "Failed on {}: {}", description, wkb);
+        }
+    }
+
     #[tokio::test]
     async fn test_st_isempty_null() {
         let ctx = SessionContext::new();