paleolimbot commented on code in PR #990:
URL: https://github.com/apache/sedona-db/pull/990#discussion_r3453065863
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -750,6 +750,23 @@ def test_st_buffer_style_parameters(
)
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("geom", "expected"),
+ [
+ (None, None),
+ ("LINESTRING (0 0, 1 0, 1 1, 0 0)", "POLYGON ((0 0, 1 1, 1 0, 0 0))"),
+ (
+ "MULTILINESTRING ((0 0, 1 0, 1 1, 0 0), (2 2, 3 2, 3 3, 2 2))",
+ "MULTIPOLYGON (((1 1, 1 0, 0 0, 1 1)), ((3 3, 3 2, 2 2, 3 3)))",
+ ),
+ ],
Review Comment:
Can you add cases here for LINESTRING EMPTY and MULTILINESTRING EMPTY?
Can you a separate test to confirm what happens when a POINT or POLYGON
arrives in this function? (I am guessing an error. You may have to do something
like
```python
with pytest.raises(..., match="(sedona_error)|(postgis_error)"):`
```
...since the message might be slightly different.
We may have to decide how we handle non linestring EMPTY here (some PostGIS
functions will error for an EMPTY of the wrong geometry type...in general in
SedonaDB we handle them all the same, so an input of POINT EMPTY would give the
same output as a LINESTRING EMPTY here).
##########
docs/reference/sql/st_exteriorring.qmd:
##########
@@ -0,0 +1,41 @@
+---
+# 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.
+
+title: ST_ExteriorRing
+description: Returns the exterior ring of a polygon as a linear ring geometry.
+kernels:
+ - returns: geometry
+ args: [geometry]
+ - returns: geography
+ args: [geography]
+---
+
+## Description
+
+Returns the exterior ring (outer boundary) of a polygon as a `LINEARRING`.
Returns NULL for
+non-polygon geometry types.
+
+## Examples
+
+```sql
+SELECT ST_ExteriorRing(ST_GeomFromWKT('POLYGON ((0 0, 4 0, 4 4, 0 4, 0 0), (1
1, 1 3, 3 3, 3 1, 1 1))'));
+```
+
+```sql
+SELECT ST_ExteriorRing(ST_GeogFromWKT('POLYGON ((0 0, 4 0, 4 4, 0 4, 0 0))'));
+```
Review Comment:
Can you align these examples exactly so that the difference (geom vs geog)
is more obvious?
##########
docs/reference/sql/st_buildarea.qmd:
##########
@@ -0,0 +1,42 @@
+---
+# 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.
+
+title: ST_BuildArea
+description: Returns a geometry that encloses the area formed by the given
linework.
+kernels:
+ - returns: geometry
+ args: [geometry]
+ - returns: geography
+ args: [geography]
+---
+
+## Description
+
+Creates an areal geometry from linework. The input may be any collection of
lines, polygons,
+or points. Lines are used to build the polygon boundary, and any unclosed
lines are automatically
+closed. Returns NULL for non-polygon input that produces no area.
Review Comment:
Can you rewrite this in your own words? (Parts of this are a verbatim copy
of the PostGIS documentation)
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -1421,6 +1438,28 @@ def test_st_dump(eng):
assert actual["geom"] == shapely.from_wkt(expected["geom"]).wkb
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("geom", "expected"),
+ [
+ (None, None),
+ (
+ "POLYGON ((0 0, 1 0, 0.5 1, 0 0))",
+ "GEOMETRYCOLLECTION (POLYGON ((0.5 1, 0 0, 1 0, 0.5 1)))",
+ ),
Review Comment:
Can you add cases for EMPTY input and LINESTRING input here?
Can you add a separate test with similar cases for the tolerance parameter?
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -2659,6 +2721,26 @@ def test_st_pointn(eng, geometry, n, expected):
)
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("geom", "expected"),
+ [
+ (None, None),
+ ("POLYGON ((0 0, 4 0, 4 4, 0 4, 0 0))", "POINT (2 2)"),
+ (
+ "POLYGON ((0 0, 4 0, 4 4, 0 4, 0 0), (1 1, 1 2, 2 2, 2 1, 1 1))",
+ "POINT (2 3)",
+ ),
Review Comment:
Can you add cases for EMPTY input?
Can you cases for non-empty MULTIPOINT, MULTILINESTRING, and
GEOMETRYCOLLECTION input?
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -1451,6 +1490,29 @@ def test_st_envelope(eng, geom, expected):
eng.assert_query_result(f"SELECT ST_Envelope({geom_or_null(geom)})",
expected)
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("geom", "expected"),
+ [
+ (None, None),
+ (
+ "POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0))",
+ "LINESTRING (0 0, 1 0, 1 1, 0 1, 0 0)",
+ ),
+ (
Review Comment:
Can you add cases for EMPTY input?
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -3767,6 +3849,28 @@ def test_st_numinteriorrings_basic(eng, geom, expected):
)
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("geom", "expected"),
+ [
+ (None, None),
+ ("POLYGON ((0 0, 4 0, 4 4, 0 4, 0 0))", 0),
+ (
+ "POLYGON ((0 0,6 0,6 6,0 6,0 0),(2 2,4 2,4 4,2 4,2 2))",
+ 1,
+ ),
+ ("POINT (0 0)", None),
+ ("LINESTRING (0 0, 1 1)", None),
+ ],
+)
+def test_st_numinteriorring(eng, geom, expected):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ f"SELECT ST_NumInteriorRing({geom_or_null(geom)})",
+ expected,
+ )
Review Comment:
Because this is a true alias, we should just add another assert_query_result
to the existing test for this function (ensures exact behaviour for the exact
same cases)
##########
c/sedona-geos/src/st_delaunaytriangles.rs:
##########
@@ -0,0 +1,152 @@
+// 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::BinaryBuilder;
+use arrow_schema::DataType;
+use datafusion_common::{cast::as_float64_array, DataFusionError, Result};
+use datafusion_expr::ColumnarValue;
+use geos::{Geom, Geometry};
+use sedona_expr::{
+ item_crs::ItemCrsKernel,
+ scalar_udf::{ScalarKernelRef, SedonaScalarKernel},
+};
+use sedona_geometry::wkb_factory::WKB_MIN_PROBABLE_BYTES;
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+
+use crate::executor::GeosExecutor;
+use crate::geos_to_wkb::write_geos_geometry;
+
+/// ST_DelaunayTriangles(geom) — no tolerance, returns polygons
+pub fn st_delaunay_triangles_impl() -> Vec<ScalarKernelRef> {
+ ItemCrsKernel::wrap_impl(STDelaunayTriangles { tolerance: false })
+}
+
+/// ST_DelaunayTriangles(geom, tolerance) — with tolerance
+pub fn st_delaunay_triangles_tolerance_impl() -> Vec<ScalarKernelRef> {
+ ItemCrsKernel::wrap_impl(STDelaunayTriangles { tolerance: true })
+}
+
+#[derive(Debug)]
+struct STDelaunayTriangles {
+ tolerance: bool,
+}
+
+impl SedonaScalarKernel for STDelaunayTriangles {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let arg_matchers = if self.tolerance {
+ vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()]
+ } else {
+ vec![ArgMatcher::is_geometry()]
+ };
+ let matcher = ArgMatcher::new(arg_matchers, WKB_GEOMETRY);
+ matcher.match_args(args)
Review Comment:
Can you implement this using two separate `SedonaScalarKernel`
implementations rather than crunching them both into the same one? You can
split out the logic into a common `invoke_scalar()`.
While you're here, is it easy to add support for the "flags" parameter? We
can't support the 2 (TIN) option but I believe GEOS has the MULTILINESTRING
output option.
--
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]
