petern48 commented on code in PR #219: URL: https://github.com/apache/sedona-db/pull/219#discussion_r2434368722
########## rust/sedona-functions/src/st_isclosed.rs: ########## @@ -0,0 +1,175 @@ +// 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. + +use std::sync::Arc; + +use arrow_array::builder::BooleanBuilder; +use arrow_schema::DataType; +use datafusion_common::error::Result; +use datafusion_expr::{scalar_doc_sections::DOC_SECTION_OTHER, Documentation, Volatility}; +use geo_traits::{ + to_geo::{ToGeoGeometryCollection, ToGeoLineString, ToGeoMultiLineString}, + GeometryTrait, +}; +use sedona_expr::scalar_udf::{SedonaScalarKernel, SedonaScalarUDF}; +use sedona_schema::{datatypes::SedonaType, matchers::ArgMatcher}; +use wkb::reader::Wkb; + +use crate::executor::WkbExecutor; + +pub fn st_isclosed_udf() -> SedonaScalarUDF { + SedonaScalarUDF::new( + "st_isclosed", + vec![Arc::new(STIsClosed {})], + Volatility::Immutable, + Some(st_is_closed_doc()), + ) +} + +fn st_is_closed_doc() -> Documentation { + Documentation::builder( + DOC_SECTION_OTHER, + "Return true if the geometry is closed", + "ST_IsClosed (A: Geometry)", + ) + .with_argument("geom", "geometry: Input geometry") + .with_sql_example("SELECT ST_IsClosed(ST_GeomFromWKT('LINESTRING(0 0, 1 1, 0 1, 0 0)'))") + .build() +} + +#[derive(Debug)] +struct STIsClosed {} + +impl SedonaScalarKernel for STIsClosed { + fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> { + let matcher = ArgMatcher::new( + vec![ArgMatcher::is_geometry()], + SedonaType::Arrow(DataType::Boolean), + ); + + matcher.match_args(args) + } + + fn invoke_batch( + &self, + arg_types: &[SedonaType], + args: &[datafusion_expr::ColumnarValue], + ) -> Result<datafusion_expr::ColumnarValue> { + let executor = WkbExecutor::new(arg_types, args); + let mut builder = BooleanBuilder::with_capacity(executor.num_iterations()); + + executor.execute_wkb_void(|maybe_item| { + match maybe_item { + Some(item) => { + builder.append_value(invoke_scalar(&item)?); + } + None => builder.append_null(), + } + Ok(()) + })?; + + executor.finish(Arc::new(builder.finish())) + } +} + +fn invoke_scalar(item: &Wkb) -> Result<bool> { + is_geometry_closed(item).map_err(|e| { + datafusion_common::error::DataFusionError::Execution(format!( + "Failed to check if geometry is closed: {e}" + )) + }) +} + +fn is_geometry_closed<G: GeometryTrait<T = f64>>(item: G) -> Result<bool> { + match item.as_type() { + geo_traits::GeometryType::LineString(linestring) => { + Ok(linestring.to_line_string().is_closed()) + } + geo_traits::GeometryType::MultiLineString(multilinestring) => { + Ok(multilinestring.to_multi_line_string().is_closed()) + } + geo_traits::GeometryType::GeometryCollection(geometry_collection) => geometry_collection + .to_geometry_collection() + .iter() + .try_fold(true, |acc, item| { + is_geometry_closed(item).map(|is_closed| acc && is_closed) + }), + geo_traits::GeometryType::Point(_) + | geo_traits::GeometryType::Line(_) + | geo_traits::GeometryType::MultiPoint(_) + | geo_traits::GeometryType::Polygon(_) + | geo_traits::GeometryType::MultiPolygon(_) + | geo_traits::GeometryType::Rect(_) + | geo_traits::GeometryType::Triangle(_) => Ok(true), + } +} + +#[cfg(test)] +mod tests { + use arrow_array::{create_array as arrow_array, ArrayRef}; + use datafusion_expr::ScalarUDF; + use rstest::rstest; + use sedona_schema::datatypes::{WKB_GEOMETRY, WKB_VIEW_GEOMETRY}; + use sedona_testing::{compare::assert_array_equal, testers::ScalarUdfTester}; + + use super::*; + + #[test] + fn udf_metadata() { + let udf: ScalarUDF = st_isclosed_udf().into(); + assert_eq!(udf.name(), "st_isclosed"); + assert!(udf.documentation().is_some()); + } + + #[rstest] + fn udf(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType) { + use datafusion_common::ScalarValue; + + let tester = ScalarUdfTester::new(st_isclosed_udf().into(), vec![sedona_type.clone()]); + + tester.assert_return_type(DataType::Boolean); + + let result = tester + .invoke_wkb_scalar(Some("LINESTRING(0 0, 1 1, 0 1, 0 0)")) + .unwrap(); + tester.assert_scalar_result_equals(result, ScalarValue::Boolean(Some(true))); + + let result = tester.invoke_wkb_scalar(None).unwrap(); + tester.assert_scalar_result_equals(result, ScalarValue::Null); + + let input_wkt = vec![ + None, + Some("LINESTRING(0 0, 1 1)"), + Some("LINESTRING(0 0, 0 1, 1 1, 0 0)"), + Some("MULTILINESTRING((0 0, 0 1, 1 1, 0 0),(0 0, 1 1))"), + Some("POINT(0 0)"), + Some("MULTIPOINT((0 0), (1 1))"), Review Comment: ```suggestion Some("MULTIPOINT((0 0), (1 1))"), Some("LINESTRING EMPTY"), Some("POINT EMPTY"), ``` optional nit: The Rust tests don't need to be nearly as comprehensive as the Python ones, but I think it would be nice to at least have one test for the empty cases, since it's a non-neglible part of the implementation. ########## rust/sedona-functions/src/st_isclosed.rs: ########## @@ -0,0 +1,175 @@ +// 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. + +use std::sync::Arc; + +use arrow_array::builder::BooleanBuilder; +use arrow_schema::DataType; +use datafusion_common::error::Result; +use datafusion_expr::{scalar_doc_sections::DOC_SECTION_OTHER, Documentation, Volatility}; +use geo_traits::{ + to_geo::{ToGeoGeometryCollection, ToGeoLineString, ToGeoMultiLineString}, + GeometryTrait, +}; +use sedona_expr::scalar_udf::{SedonaScalarKernel, SedonaScalarUDF}; +use sedona_schema::{datatypes::SedonaType, matchers::ArgMatcher}; +use wkb::reader::Wkb; + +use crate::executor::WkbExecutor; + +pub fn st_isclosed_udf() -> SedonaScalarUDF { + SedonaScalarUDF::new( + "st_isclosed", + vec![Arc::new(STIsClosed {})], + Volatility::Immutable, + Some(st_is_closed_doc()), + ) +} + +fn st_is_closed_doc() -> Documentation { + Documentation::builder( + DOC_SECTION_OTHER, + "Return true if the geometry is closed", + "ST_IsClosed (A: Geometry)", + ) + .with_argument("geom", "geometry: Input geometry") + .with_sql_example("SELECT ST_IsClosed(ST_GeomFromWKT('LINESTRING(0 0, 1 1, 0 1, 0 0)'))") + .build() +} + +#[derive(Debug)] +struct STIsClosed {} + +impl SedonaScalarKernel for STIsClosed { + fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> { + let matcher = ArgMatcher::new( + vec![ArgMatcher::is_geometry()], + SedonaType::Arrow(DataType::Boolean), + ); + + matcher.match_args(args) + } + + fn invoke_batch( + &self, + arg_types: &[SedonaType], + args: &[datafusion_expr::ColumnarValue], + ) -> Result<datafusion_expr::ColumnarValue> { + let executor = WkbExecutor::new(arg_types, args); + let mut builder = BooleanBuilder::with_capacity(executor.num_iterations()); + + executor.execute_wkb_void(|maybe_item| { + match maybe_item { + Some(item) => { + builder.append_value(invoke_scalar(&item)?); + } + None => builder.append_null(), + } + Ok(()) + })?; + + executor.finish(Arc::new(builder.finish())) + } +} + +fn invoke_scalar(item: &Wkb) -> Result<bool> { + is_geometry_closed(item).map_err(|e| { + datafusion_common::error::DataFusionError::Execution(format!( + "Failed to check if geometry is closed: {e}" + )) + }) Review Comment: ```suggestion is_geometry_closed(item) ``` `.map_err()` is usually only needed to convert the Error type of the result from one error type to `DataFusionError` (e.g the `is_geometry_empty` returns `Result<_, SedonaGeometryError>`, so we need to call it for that below) In this case, both `is_geometry_closed` and `invoke_scalar` return the DataFusionError, so there's no need to convert ########## python/sedonadb/tests/functions/test_functions.py: ########## @@ -573,6 +573,23 @@ def test_st_isempty(eng, geom, expected): eng.assert_query_result(f"SELECT ST_IsEmpty({geom_or_null(geom)})", expected) [email protected]("eng", [SedonaDB, PostGIS]) [email protected]( + ("geom", "expected"), + [ + (None, None), + ("LINESTRING(0 0, 1 1)", False), + ("LINESTRING(0 0, 0 1, 1 1, 0 0)", True), + ("MULTILINESTRING((0 0, 0 1, 1 1, 0 0),(0 0, 1 1))", False), + ("POINT(0 0)", True), + ("MULTIPOINT((0 0), (1 1))", True), Review Comment: ```suggestion ("MULTIPOINT((0 0), (1 1))", True), ("POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0))", True), ("GEOMETRYCOLLECTION (LINESTRING(0 0, 0 1, 1 1, 0 0))", True), ("GEOMETRYCOLLECTION (LINESTRING(0 0, 0 1, 1 1, 0 0), LINESTRING(0 0, 1 1))", False), ("POINT EMPTY", False), ("LINESTRING EMPTY", False), ("POLYGON EMPTY", False), ("MULTIPOINT EMPTY", False), ("MULTILINESTRING EMPTY", False), ("MULTIPOLYGON EMPTY", False), ("GEOMETRYCOLLECTION EMPTY", False), ("GEOMETRYCOLLECTION (LINESTRING EMPTY)", False), ``` Since we're implementing more of this logic manually, we should test more comprehensively. Here are some more cases I suggest we add. - One non-empty geometry for each of the 7 types - GeometryCollection with a closed linestring and an open linestring (False) - Empty geometry for each of the 7 types - Non-empty GeometryCollection containing only empties ########## rust/sedona-functions/src/st_isclosed.rs: ########## @@ -0,0 +1,175 @@ +// 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. + +use std::sync::Arc; + +use arrow_array::builder::BooleanBuilder; +use arrow_schema::DataType; +use datafusion_common::error::Result; +use datafusion_expr::{scalar_doc_sections::DOC_SECTION_OTHER, Documentation, Volatility}; +use geo_traits::{ + to_geo::{ToGeoGeometryCollection, ToGeoLineString, ToGeoMultiLineString}, + GeometryTrait, +}; +use sedona_expr::scalar_udf::{SedonaScalarKernel, SedonaScalarUDF}; +use sedona_schema::{datatypes::SedonaType, matchers::ArgMatcher}; +use wkb::reader::Wkb; + +use crate::executor::WkbExecutor; + +pub fn st_isclosed_udf() -> SedonaScalarUDF { + SedonaScalarUDF::new( + "st_isclosed", + vec![Arc::new(STIsClosed {})], + Volatility::Immutable, + Some(st_is_closed_doc()), + ) +} + +fn st_is_closed_doc() -> Documentation { + Documentation::builder( + DOC_SECTION_OTHER, + "Return true if the geometry is closed", + "ST_IsClosed (A: Geometry)", + ) + .with_argument("geom", "geometry: Input geometry") + .with_sql_example("SELECT ST_IsClosed(ST_GeomFromWKT('LINESTRING(0 0, 1 1, 0 1, 0 0)'))") + .build() +} + +#[derive(Debug)] +struct STIsClosed {} + +impl SedonaScalarKernel for STIsClosed { + fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> { + let matcher = ArgMatcher::new( + vec![ArgMatcher::is_geometry()], + SedonaType::Arrow(DataType::Boolean), + ); + + matcher.match_args(args) + } + + fn invoke_batch( + &self, + arg_types: &[SedonaType], + args: &[datafusion_expr::ColumnarValue], + ) -> Result<datafusion_expr::ColumnarValue> { + let executor = WkbExecutor::new(arg_types, args); + let mut builder = BooleanBuilder::with_capacity(executor.num_iterations()); + + executor.execute_wkb_void(|maybe_item| { + match maybe_item { + Some(item) => { + builder.append_value(invoke_scalar(&item)?); + } + None => builder.append_null(), + } + Ok(()) + })?; + + executor.finish(Arc::new(builder.finish())) + } +} + +fn invoke_scalar(item: &Wkb) -> Result<bool> { + is_geometry_closed(item).map_err(|e| { + datafusion_common::error::DataFusionError::Execution(format!( + "Failed to check if geometry is closed: {e}" + )) + }) +} + +fn is_geometry_closed<G: GeometryTrait<T = f64>>(item: G) -> Result<bool> { + match item.as_type() { + geo_traits::GeometryType::LineString(linestring) => { + Ok(linestring.to_line_string().is_closed()) + } + geo_traits::GeometryType::MultiLineString(multilinestring) => { + Ok(multilinestring.to_multi_line_string().is_closed()) + } + geo_traits::GeometryType::GeometryCollection(geometry_collection) => geometry_collection + .to_geometry_collection() + .iter() + .try_fold(true, |acc, item| { + is_geometry_closed(item).map(|is_closed| acc && is_closed) + }), + geo_traits::GeometryType::Point(_) + | geo_traits::GeometryType::Line(_) + | geo_traits::GeometryType::MultiPoint(_) + | geo_traits::GeometryType::Polygon(_) + | geo_traits::GeometryType::MultiPolygon(_) + | geo_traits::GeometryType::Rect(_) + | geo_traits::GeometryType::Triangle(_) => Ok(true), Review Comment: ```suggestion geo_traits::GeometryType::Point(_) | geo_traits::GeometryType::MultiPoint(_) | geo_traits::GeometryType::Polygon(_) | geo_traits::GeometryType::MultiPolygon(_) => Ok(true), _ => sedona_internal_err!("Invalid geometry type"), ``` SedonaDB doesn't support `Line`, `Rect`, or `Triangle` geometry types, so I think it's better to omit them and error on the default case (this is what we do in other functions). If we somehow got those geom types, I'd rather not hide that bug. ########## python/sedonadb/.gitignore: ########## @@ -1,88 +1,2 @@ -# Licensed to the Apache Software Foundation (ASF) under one Review Comment: Let's undo this change to `.gitignore` ########## rust/sedona-functions/src/st_isclosed.rs: ########## @@ -0,0 +1,175 @@ +// 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. + +use std::sync::Arc; + +use arrow_array::builder::BooleanBuilder; +use arrow_schema::DataType; +use datafusion_common::error::Result; +use datafusion_expr::{scalar_doc_sections::DOC_SECTION_OTHER, Documentation, Volatility}; +use geo_traits::{ + to_geo::{ToGeoGeometryCollection, ToGeoLineString, ToGeoMultiLineString}, + GeometryTrait, +}; +use sedona_expr::scalar_udf::{SedonaScalarKernel, SedonaScalarUDF}; +use sedona_schema::{datatypes::SedonaType, matchers::ArgMatcher}; +use wkb::reader::Wkb; + +use crate::executor::WkbExecutor; + +pub fn st_isclosed_udf() -> SedonaScalarUDF { + SedonaScalarUDF::new( + "st_isclosed", + vec![Arc::new(STIsClosed {})], + Volatility::Immutable, + Some(st_is_closed_doc()), + ) +} + +fn st_is_closed_doc() -> Documentation { + Documentation::builder( + DOC_SECTION_OTHER, + "Return true if the geometry is closed", + "ST_IsClosed (A: Geometry)", + ) + .with_argument("geom", "geometry: Input geometry") + .with_sql_example("SELECT ST_IsClosed(ST_GeomFromWKT('LINESTRING(0 0, 1 1, 0 1, 0 0)'))") + .build() +} + +#[derive(Debug)] +struct STIsClosed {} + +impl SedonaScalarKernel for STIsClosed { + fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> { + let matcher = ArgMatcher::new( + vec![ArgMatcher::is_geometry()], + SedonaType::Arrow(DataType::Boolean), + ); + + matcher.match_args(args) + } + + fn invoke_batch( + &self, + arg_types: &[SedonaType], + args: &[datafusion_expr::ColumnarValue], + ) -> Result<datafusion_expr::ColumnarValue> { + let executor = WkbExecutor::new(arg_types, args); + let mut builder = BooleanBuilder::with_capacity(executor.num_iterations()); + + executor.execute_wkb_void(|maybe_item| { + match maybe_item { + Some(item) => { + builder.append_value(invoke_scalar(&item)?); + } + None => builder.append_null(), + } + Ok(()) + })?; + + executor.finish(Arc::new(builder.finish())) + } +} + +fn invoke_scalar(item: &Wkb) -> Result<bool> { + is_geometry_closed(item).map_err(|e| { + datafusion_common::error::DataFusionError::Execution(format!( + "Failed to check if geometry is closed: {e}" + )) + }) +} + +fn is_geometry_closed<G: GeometryTrait<T = f64>>(item: G) -> Result<bool> { Review Comment: ```suggestion fn is_geometry_closed<G: GeometryTrait<T = f64>>(item: G) -> Result<bool> { if is_geometry_empty(&item).map_err(|e| { datafusion_common::error::DataFusionError::Execution(format!( "Failed to check if geometry is empty: {e}" )) })? { return Ok(false); } ``` I just queried PostGIS locally, it seems like it returns `false` for any empty geometry, so we should use the Sedona's `is_geometry_empty` function. The import for it is below. Note: I originally placed this inside of the final match arm below, but it turns out `geo`'s `is_closed()` methods return `true` for empty linestrings (which is not what we want), so we need to have this check above the match statement. Otherwise, we'll return the wrong result for empty linestring, multilinestring, and geomcol. ```rust use sedona_geometry::is_empty::is_geometry_empty; ``` -- 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]
