petern48 commented on code in PR #409:
URL: https://github.com/apache/sedona-db/pull/409#discussion_r2596490108
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -2935,3 +2929,32 @@ def test_st_NRings(eng, geom, expected):
f"SELECT ST_NRings({geom_or_null(geom)})",
expected,
)
+
+
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("geom", "expected"),
+ [
+ (None, None),
Review Comment:
```suggestion
(None, None),
("POINT EMPTY", None),
("LINESTRING EMPTY", None),
("POLYGON EMPTY", 0),
("MULTIPOINT EMPTY", None),
("MULTILINESTRING EMPTY", None),
("MULTIPOLYGON EMPTY", None),
("GEOMETRYCOLLECTION EMPTY", None),
```
As always, I want to test empty versions of each of the geometry types
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -2935,3 +2929,32 @@ def test_st_NRings(eng, geom, expected):
f"SELECT ST_NRings({geom_or_null(geom)})",
expected,
)
+
+
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("geom", "expected"),
+ [
+ (None, None),
+ ("POINT (1 2)", None),
+ ("LINESTRING (0 0, 1 1, 2 2)", None),
+ ("MULTILINESTRING ((0 0, 1 1), (2 2, 3 3))", None),
+ ("POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0))", "LINESTRING (0 0, 1 0, 1 1, 0
1, 0 0)"),
+ (
+ "POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0), (1 1, 1 2, 2 2, 2 1, 1
1))",
+ "LINESTRING (0 0, 10 0, 10 10, 0 10, 0 0)",
+ ),
+ (
+ "MULTIPOLYGON (((0 0, 1 0, 1 1, 0 1, 0 0)), ((10 10, 20 10, 20 20,
10 20, 10 10)))",
+ None,
+ ),
+ ("GEOMETRYCOLLECTION(POINT(1 1), POLYGON((0 0, 1 0, 1 1, 0 0)))",
None),
+ (
+ "POLYGON Z ((0 0 1, 1 0 1, 1 1 1, 0 1 1, 0 0 1))",
+ "LINESTRING Z (0 0 1, 1 0 1, 1 1 1, 0 1 1, 0 0 1)",
Review Comment:
Can you also add `POLYGON M` and `POLYGON ZM` cases? I think your current
code might fail these, at the moment, since I see logic for `has_z` instead of
using our usual pattern.
You can `Ctrl + f` in this `test_functions.py` file and search for `POLYGON
M` and `POLYGON ZM` to find examples of test cases.
##########
c/sedona-geos/src/st_exteriorring.rs:
##########
@@ -0,0 +1,165 @@
+// 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 crate::executor::GeosExecutor;
+use arrow_array::builder::BinaryBuilder;
+use datafusion_common::{error::Result, DataFusionError};
+use datafusion_expr::ColumnarValue;
+use geos::{Geom, GeometryTypes::Polygon, OutputDimension, WKBWriter};
+use sedona_expr::scalar_udf::{ScalarKernelRef, SedonaScalarKernel};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use std::sync::Arc;
+
+pub fn st_exterior_ring_impl() -> ScalarKernelRef {
+ Arc::new(STExteriorRing {})
+}
+
+#[derive(Debug)]
+struct STExteriorRing {}
+
+impl SedonaScalarKernel for STExteriorRing {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()],
WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ 100 * executor.num_iterations(),
Review Comment:
```suggestion
WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
```
To figure out how to set values like this, I recommend looking at other
functions that return a geometry like `st_centroid.rs`. e.g.
[here](https://github.com/apache/sedona-db/blob/c48dfaa06858b5fe3ddf8d68115a890fe222cab1/c/sedona-geos/src/st_centroid.rs#L53-L56)
You'll need to add the import at the top too:
```rust
use sedona_geometry::wkb_factory::WKB_MIN_PROBABLE_BYTES;
```
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -2935,3 +2929,32 @@ def test_st_NRings(eng, geom, expected):
f"SELECT ST_NRings({geom_or_null(geom)})",
expected,
)
+
+
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("geom", "expected"),
+ [
+ (None, None),
+ ("POINT (1 2)", None),
Review Comment:
```suggestion
("POINT (1 2)", None),
("MULTIPOINT ((0 0), (1 1))", None),
```
I prefer to also test a non-empty version of each geometry type as well. You
have all except multipoint already.
##########
c/sedona-geos/src/st_exteriorring.rs:
##########
@@ -0,0 +1,165 @@
+// 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 crate::executor::GeosExecutor;
+use arrow_array::builder::BinaryBuilder;
+use datafusion_common::{error::Result, DataFusionError};
+use datafusion_expr::ColumnarValue;
+use geos::{Geom, GeometryTypes::Polygon, OutputDimension, WKBWriter};
+use sedona_expr::scalar_udf::{ScalarKernelRef, SedonaScalarKernel};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use std::sync::Arc;
+
+pub fn st_exterior_ring_impl() -> ScalarKernelRef {
+ Arc::new(STExteriorRing {})
+}
+
+#[derive(Debug)]
+struct STExteriorRing {}
+
+impl SedonaScalarKernel for STExteriorRing {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()],
WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ 100 * executor.num_iterations(),
+ );
+
+ executor.execute_wkb_void(|maybe_wkb| {
+ match maybe_wkb {
+ Some(wkb) => {
+ invoke_scalar(&wkb, &mut builder)?;
+ }
+ _ => builder.append_null(),
+ }
+
+ Ok(())
+ })?;
+
+ executor.finish(Arc::new(builder.finish()))
+ }
+}
+fn invoke_scalar(geos_geom: &geos::Geometry, builder: &mut BinaryBuilder) ->
Result<()> {
+ match geos_geom.geometry_type() {
+ Polygon => {
+ let ring = geos_geom.get_exterior_ring().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get exterior
ring: {e}"))
+ })?;
+ let mut writer = WKBWriter::new().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to create WKB
writer: {e}"))
+ })?;
+
+ if geos_geom.has_z().unwrap_or(false) {
+ writer.set_output_dimension(OutputDimension::ThreeD);
+ }
+
+ let wkb = writer.write_wkb(&ring).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert to wkb:
{e}"))
+ })?;
+
+ builder.append_value(wkb);
+ }
+ // ST_ExteriorRing is only valid for Polygons.
+ // For MultiPolygons, Points, LineStrings, etc., it returns NULL.
+ _ => builder.append_null(),
+ }
+ Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use datafusion_common::ScalarValue;
+ use rstest::rstest;
+ use sedona_expr::scalar_udf::SedonaScalarUDF;
+ use sedona_schema::datatypes::{WKB_GEOMETRY, WKB_VIEW_GEOMETRY};
+ use sedona_testing::compare::assert_array_equal;
+ use sedona_testing::create::create_array;
+ use sedona_testing::testers::ScalarUdfTester;
+
+ fn geos_wkt_to_wkb(wkt: &str) -> Vec<u8> {
+ let g = geos::Geometry::new_from_wkt(wkt).unwrap();
+ let mut writer = geos::WKBWriter::new().unwrap();
+ if g.has_z().unwrap_or(false) {
+ writer.set_output_dimension(geos::OutputDimension::ThreeD);
+ }
+ writer.write_wkb(&g).unwrap().into()
+ }
+
+ #[rstest]
+ fn udf(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType)
{
+ let udf = SedonaScalarUDF::from_kernel("st_exterior_ring",
st_exterior_ring_impl());
+ let tester = ScalarUdfTester::new(udf.into(), vec![sedona_type]);
+
+ tester.assert_return_type(WKB_GEOMETRY);
+
+ // Test 1: Standard 2D Polygon
+ let result = tester
+ .invoke_scalar("POLYGON((0 0, 1 0, 1 1, 0 0))")
+ .unwrap();
+ tester.assert_scalar_result_equals(result, "LINESTRING (0 0, 1 0, 1 1,
0 0)");
+
+ // Test 2: 3D Polygon (Z-coordinates)
+ let z_wkt_in = "POLYGON Z ((0 0 1, 1 0 1, 1 1 1, 0 1 1, 0 0 1))";
+ let z_wkt_out = "LINESTRING Z (0 0 1, 1 0 1, 1 1 1, 0 1 1, 0 0 1)";
+
+ let result_z = tester.invoke_scalar(z_wkt_in).unwrap();
+ match result_z {
+ ScalarValue::Binary(Some(bytes)) |
ScalarValue::LargeBinary(Some(bytes)) => {
+ let expected_bytes = geos_wkt_to_wkb(z_wkt_out);
+ assert_eq!(
+ bytes, expected_bytes,
+ "Z-coordinates were not preserved correctly"
+ );
+ }
+ _ => panic!("Expected Binary result for 3D test"),
+ }
Review Comment:
(Genuine question) Do we really need this extra logic for testing `POLYGON
Z` here? I would've thought we could use it in the `input_wkt` and `expected`
arrays below.
Let's remove this complexity by moving this test case into the arrays below.
If that doesn't work. Let me know.
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -2904,12 +2904,6 @@ def test_st_numpoints(eng, geom, expected):
("MULTIPOINT EMPTY", 0),
("LINESTRING EMPTY", 0),
("MULTILINESTRING EMPTY", 0),
- ("MULTIPOINT ((0 0), (1 1))", 0),
- ("MULTILINESTRING ((0 0, 1 1), (2 2, 3 3))", 0),
- ("POINT EMPTY", 0),
- ("MULTIPOINT EMPTY", 0),
- ("LINESTRING EMPTY", 0),
- ("MULTILINESTRING EMPTY", 0),
Review Comment:
Thanks for doing this. For other readers, this is just clean up from a
previous PR, as explained
[here](https://github.com/apache/sedona-db/pull/387#discussion_r2581685826)
##########
c/sedona-geos/src/st_exteriorring.rs:
##########
@@ -0,0 +1,165 @@
+// 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 crate::executor::GeosExecutor;
+use arrow_array::builder::BinaryBuilder;
+use datafusion_common::{error::Result, DataFusionError};
+use datafusion_expr::ColumnarValue;
+use geos::{Geom, GeometryTypes::Polygon, OutputDimension, WKBWriter};
+use sedona_expr::scalar_udf::{ScalarKernelRef, SedonaScalarKernel};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use std::sync::Arc;
+
+pub fn st_exterior_ring_impl() -> ScalarKernelRef {
+ Arc::new(STExteriorRing {})
+}
+
+#[derive(Debug)]
+struct STExteriorRing {}
+
+impl SedonaScalarKernel for STExteriorRing {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()],
WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ 100 * executor.num_iterations(),
+ );
+
+ executor.execute_wkb_void(|maybe_wkb| {
+ match maybe_wkb {
+ Some(wkb) => {
+ invoke_scalar(&wkb, &mut builder)?;
+ }
+ _ => builder.append_null(),
+ }
+
+ Ok(())
+ })?;
+
+ executor.finish(Arc::new(builder.finish()))
+ }
+}
+fn invoke_scalar(geos_geom: &geos::Geometry, builder: &mut BinaryBuilder) ->
Result<()> {
+ match geos_geom.geometry_type() {
+ Polygon => {
+ let ring = geos_geom.get_exterior_ring().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get exterior
ring: {e}"))
+ })?;
+ let mut writer = WKBWriter::new().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to create WKB
writer: {e}"))
+ })?;
+
+ if geos_geom.has_z().unwrap_or(false) {
+ writer.set_output_dimension(OutputDimension::ThreeD);
+ }
+
+ let wkb = writer.write_wkb(&ring).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert to wkb:
{e}"))
+ })?;
+
+ builder.append_value(wkb);
Review Comment:
While it seems like this works (though I think it would fail for `M` and
`ZM` cases), we should follow existing code patterns instead of introducing
this less concise and somewhat more complex logic.
I encourage you to look at `st_centroid.rs` to see how you can implement the
wkb writing more cleanly. Specifically look
[here](https://github.com/apache/sedona-db/blob/c48dfaa06858b5fe3ddf8d68115a890fe222cab1/c/sedona-geos/src/st_centroid.rs#L78-L82)
and how that's called
[here](https://github.com/apache/sedona-db/blob/c48dfaa06858b5fe3ddf8d68115a890fe222cab1/c/sedona-geos/src/st_centroid.rs#L59-L63)
--
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]
