You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "joonaspessi (via GitHub)" <gi...@apache.org> on 2025/10/25 10:19:48 UTC
[PR] feat(c/sedona-geos): Implement ST_SimplifyPreserveTopology using geos [sedona-db]
joonaspessi opened a new pull request, #243:
URL: https://github.com/apache/sedona-db/pull/243
Implements `ST_SimplifyPreserveTopology(geometry, tolerance)` function using geos.
**Notice about ST_Simplify**
Actually started the implementation of `ST_Simplify` but noticed that the PostGIS has different behavior for collapsed geometries and collection types in comparison to [GEOS implementation](https://libgeos.org/doxygen/classgeos_1_1simplify_1_1DouglasPeuckerSimplifier.html).
Achieving full PostGIS compatibility (including all the params) would probably require custom Rust implementation.
- Both PostGIS and Sedona-DB uses now GEOS for ST_SimplifyPreserveTopology
- https://github.com/postgis/postgis/blob/master/postgis/lwgeom_geos.c#L935 - Entry point
- https://github.com/postgis/postgis/blob/master/postgis/lwgeom_geos.c#L966 - GEOS function call
- PostGIS uses custom implementation for ST_Simplify
- https://github.com/postgis/postgis/blob/master/postgis/lwgeom_functions_analytic.c#L58 - Entry point
- https://github.com/postgis/postgis/blob/master/liblwgeom/lwgeom.c#L1851 - Core custom implementation
This might be actually nice function to be ported into Rust.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Implement ST_SimplifyPreserveTopology using geos [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #243:
URL: https://github.com/apache/sedona-db/pull/243#discussion_r2462921858
##########
c/sedona-geos/src/register.rs:
##########
@@ -57,12 +59,14 @@ pub fn scalar_kernels() -> Vec<(&'static str, ScalarKernelRef)> {
("st_length", st_length_impl()),
("st_overlaps", st_overlaps_impl()),
("st_perimeter", st_perimeter_impl()),
+ (
+ "st_simplifypreservetopology",
+ st_simplify_preserve_topology_impl(),
+ ),
("st_symdifference", st_sym_difference_impl()),
("st_touches", st_touches_impl()),
("st_unaryunion", st_unary_union_impl()),
("st_union", st_union_impl()),
("st_within", st_within_impl()),
- ("st_crosses", st_crosses_impl()),
- ("st_overlaps", st_overlaps_impl()),
Review Comment:
Let's add these function registrations back. We shouldn't be removing any existing functionality.
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -1399,3 +1399,49 @@ def test_st_isvalidreason(eng, geom, expected):
else:
query = f"SELECT ST_IsValidReason({geom_or_null(geom)})"
eng.assert_query_result(query, expected)
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "tolerance", "expected"),
+ [
+ # Basic linestring simplification
+ (
+ "LINESTRING (0 0, 0 10, 0 51, 50 20, 30 20, 7 32)",
+ 2,
+ "LINESTRING(0 0,0 51,50 20,30 20,7 32)",
+ ),
+ # Short linestring preserves endpoints
+ (
+ "LINESTRING (0 0, 0 10)",
+ 20,
+ "LINESTRING(0 0,0 10)",
+ ),
+ # Null handling
+ (None, 2, None),
+ (None, None, None),
+ ("LINESTRING (0 0, 0 10)", None, None),
+ # Empty geometries
+ ("LINESTRING EMPTY", 2, "LINESTRING EMPTY"),
+ ("POINT EMPTY", 2, "POINT EMPTY"),
+ ("POLYGON EMPTY", 2, "POLYGON EMPTY"),
+ # POLYGON with hole - topology preserved, both rings simplified
+ (
+ "POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0),(5 5, 5 6, 6 6, 8 5, 5 5))",
+ 20,
+ "POLYGON((0 0,10 0,10 10,0 10,0 0),(5 5,5 6,8 5,5 5))",
+ ),
+ # MULTIPOLYGON - both polygons preserved with topology preservation
+ (
+ "MULTIPOLYGON (((100 100, 100 130, 130 130, 130 100, 100 100)),((0 0, 10 0, 10 10, 0 10, 0 0),(5 5, 5 6, 6 6, 8 5, 5 5)))",
+ 20,
+ "MULTIPOLYGON(((100 100,100 130,130 130,130 100,100 100)),((0 0,10 0,10 10,0 10,0 0),(5 5,5 6,8 5,5 5)))",
+ ),
+ ],
+)
+def test_st_simplifypreservetopology(eng, geom, tolerance, expected):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ f"SELECT ST_AsText(ST_SimplifyPreserveTopology({geom_or_null(geom)}, {val_or_null(tolerance)}))",
Review Comment:
optional: I was curious why `ST_AsText()` was there, so I tried removing it myself. We can actually get this to pass without it. After removing the `ST_AsText()`, the cases fail because your spacing is a bit off. It should pass if you add a space after each comma (`,`) and after the polygon name. For example, `LINESTRING(0 0,0 10)` would become `LINESTRING (0 0, 0 10)`. Another benefit is readability from having consistent spacing, as I mentioned above.
The one caveat is `POINT EMPTY`. In that case, we can set expected to `POINT (nan nan)` because that's how geoarrow-c renders it.
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -1399,3 +1399,49 @@ def test_st_isvalidreason(eng, geom, expected):
else:
query = f"SELECT ST_IsValidReason({geom_or_null(geom)})"
eng.assert_query_result(query, expected)
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "tolerance", "expected"),
+ [
+ # Basic linestring simplification
+ (
+ "LINESTRING (0 0, 0 10, 0 51, 50 20, 30 20, 7 32)",
+ 2,
+ "LINESTRING(0 0,0 51,50 20,30 20,7 32)",
+ ),
+ # Short linestring preserves endpoints
+ (
+ "LINESTRING (0 0, 0 10)",
+ 20,
+ "LINESTRING(0 0,0 10)",
+ ),
+ # Null handling
+ (None, 2, None),
+ (None, None, None),
+ ("LINESTRING (0 0, 0 10)", None, None),
+ # Empty geometries
+ ("LINESTRING EMPTY", 2, "LINESTRING EMPTY"),
+ ("POINT EMPTY", 2, "POINT EMPTY"),
+ ("POLYGON EMPTY", 2, "POLYGON EMPTY"),
+ # POLYGON with hole - topology preserved, both rings simplified
+ (
+ "POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0),(5 5, 5 6, 6 6, 8 5, 5 5))",
+ 20,
+ "POLYGON((0 0,10 0,10 10,0 10,0 0),(5 5,5 6,8 5,5 5))",
Review Comment:
nit: The comment sounds misleading. The inner ring is simplified (removing the `6 6` coordinate), but the outer ring is left the same.
It would also be helpful if we kept the spacing consistent between the input and the expected string (this applies to all of these test cases). It visually makes it easier for readers to compare the geometries and see if there's a missing coordinate or not, since our eyes can just look up-down. I found myself "restarting" my comparison one or twice after losing track of which coordinate I was on, since my eyes had to track diagonally between the top and bottom.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Implement ST_SimplifyPreserveTopology using geos [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #243:
URL: https://github.com/apache/sedona-db/pull/243#discussion_r2462946760
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -1399,3 +1399,49 @@ def test_st_isvalidreason(eng, geom, expected):
else:
query = f"SELECT ST_IsValidReason({geom_or_null(geom)})"
eng.assert_query_result(query, expected)
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "tolerance", "expected"),
+ [
+ # Basic linestring simplification
+ (
+ "LINESTRING (0 0, 0 10, 0 51, 50 20, 30 20, 7 32)",
+ 2,
+ "LINESTRING(0 0,0 51,50 20,30 20,7 32)",
+ ),
+ # Short linestring preserves endpoints
+ (
+ "LINESTRING (0 0, 0 10)",
+ 20,
+ "LINESTRING(0 0,0 10)",
+ ),
+ # Null handling
+ (None, 2, None),
+ (None, None, None),
+ ("LINESTRING (0 0, 0 10)", None, None),
+ # Empty geometries
+ ("LINESTRING EMPTY", 2, "LINESTRING EMPTY"),
+ ("POINT EMPTY", 2, "POINT EMPTY"),
+ ("POLYGON EMPTY", 2, "POLYGON EMPTY"),
+ # POLYGON with hole - topology preserved, both rings simplified
+ (
+ "POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0),(5 5, 5 6, 6 6, 8 5, 5 5))",
+ 20,
+ "POLYGON((0 0,10 0,10 10,0 10,0 0),(5 5,5 6,8 5,5 5))",
Review Comment:
Thanks for the suggestion, the consistent spacing indeed made the test case much more easier to follow and understand. Also fixed that comment that was wrongly stating that both outer and inner rings would be simplified.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Implement ST_SimplifyPreserveTopology using geos [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #243:
URL: https://github.com/apache/sedona-db/pull/243
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Implement ST_SimplifyPreserveTopology using geos [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #243:
URL: https://github.com/apache/sedona-db/pull/243#discussion_r2463837677
##########
c/sedona-geos/src/st_simplifypreservetopology.rs:
##########
@@ -0,0 +1,203 @@
+// 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::error::Result;
+use datafusion_common::DataFusionError;
+use datafusion_expr::ColumnarValue;
+use geos::Geom;
+use sedona_expr::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;
+
+/// ST_SimplifyPreserveTopology() implementation using the geos crate
+pub fn st_simplify_preserve_topology_impl() -> ScalarKernelRef {
+ Arc::new(STSimplifyPreserveTopology {})
+}
+
+#[derive(Debug)]
+struct STSimplifyPreserveTopology {}
+
+impl SedonaScalarKernel for STSimplifyPreserveTopology {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()],
+ WKB_GEOMETRY,
+ );
+
+ matcher.match_args(args)
+ }
+
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ let tolerance: Option<f64>;
+ let arg1 = args[1].cast_to(&DataType::Float64, None)?;
+ if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
+ if scalar_arg.is_null() {
+ tolerance = None;
+ } else {
+ tolerance = Some(f64::try_from(scalar_arg.clone())?);
+ }
+ } else {
+ return Err(DataFusionError::Execution(format!(
+ "Invalid tolerance: {:?}",
+ args[1]
+ )));
+ }
Review Comment:
Thanks for pointing this out, nice that the array input was relatively easy to handle with ColumnarValue. Implementation is now updated with this change.
Btw. Does this introduce some room for performance/memory improvement? With this approach and using scalar value, we will copy it for all geometries, potentially quite many times.
Thinking if these tracks should be separated, or might be just micro optimization...
```rust
let executor = GeosExecutor::new(arg_types, args);
let tolerance_arg = args[1].cast_to(&DataType::Float64, None)?;
let mut builder = BinaryBuilder::with_capacity(
executor.num_iterations(),
WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
);
match tolerance_arg {
ColumnarValue::Scalar(scalar_val) => {
// SCALAR PATH: Extract scalar once, reuse for all geometries
...
}
ColumnarValue::Array(_) => {
// ARRAY PATH: Use iterator for per-row tolerances
...
}
}
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Implement ST_SimplifyPreserveTopology using geos [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #243:
URL: https://github.com/apache/sedona-db/pull/243#discussion_r2462948601
##########
c/sedona-geos/src/register.rs:
##########
@@ -57,12 +59,14 @@ pub fn scalar_kernels() -> Vec<(&'static str, ScalarKernelRef)> {
("st_length", st_length_impl()),
("st_overlaps", st_overlaps_impl()),
Review Comment:
Already defined here
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Implement ST_SimplifyPreserveTopology using geos [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #243:
URL: https://github.com/apache/sedona-db/pull/243#discussion_r2463152774
##########
c/sedona-geos/src/st_simplifypreservetopology.rs:
##########
@@ -0,0 +1,203 @@
+// 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::error::Result;
+use datafusion_common::DataFusionError;
+use datafusion_expr::ColumnarValue;
+use geos::Geom;
+use sedona_expr::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;
+
+/// ST_SimplifyPreserveTopology() implementation using the geos crate
+pub fn st_simplify_preserve_topology_impl() -> ScalarKernelRef {
+ Arc::new(STSimplifyPreserveTopology {})
+}
+
+#[derive(Debug)]
+struct STSimplifyPreserveTopology {}
+
+impl SedonaScalarKernel for STSimplifyPreserveTopology {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()],
+ WKB_GEOMETRY,
+ );
+
+ matcher.match_args(args)
+ }
+
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ let tolerance: Option<f64>;
+ let arg1 = args[1].cast_to(&DataType::Float64, None)?;
+ if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
+ if scalar_arg.is_null() {
+ tolerance = None;
+ } else {
+ tolerance = Some(f64::try_from(scalar_arg.clone())?);
+ }
+ } else {
+ return Err(DataFusionError::Execution(format!(
+ "Invalid tolerance: {:?}",
+ args[1]
+ )));
+ }
Review Comment:
While you're here, I think it would be good to support this as an array input as well (it's roughly the same amount of code). The example I wrote up for `ST_Buffer()` was:
```rust
let distance_value = args[1].cast_to(&DataType::Float64)?.to_array(executor.num_iterations());
let distance_array = if let ColumnarValue::Array(array_ref) = distance_value {
as_float64_array(array_ref)?
} else {
return sedona_internal_err!("Unexpected scalar value");
};
let mut distance_iter = distance_array.iter();
// ...
executor.execute_wkb_void(|wkb| {
match (wkb, distance_iter.next().unwrap() {
(Some(wkb_item), Some(distance_item)) => { invoke_scalar(...) }
_ => { /* append null */ }
}
})
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Implement ST_SimplifyPreserveTopology using geos [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #243:
URL: https://github.com/apache/sedona-db/pull/243#discussion_r2462948311
##########
c/sedona-geos/src/register.rs:
##########
@@ -57,12 +59,14 @@ pub fn scalar_kernels() -> Vec<(&'static str, ScalarKernelRef)> {
("st_length", st_length_impl()),
("st_overlaps", st_overlaps_impl()),
("st_perimeter", st_perimeter_impl()),
+ (
+ "st_simplifypreservetopology",
+ st_simplify_preserve_topology_impl(),
+ ),
("st_symdifference", st_sym_difference_impl()),
("st_touches", st_touches_impl()),
("st_unaryunion", st_unary_union_impl()),
("st_union", st_union_impl()),
("st_within", st_within_impl()),
- ("st_crosses", st_crosses_impl()),
- ("st_overlaps", st_overlaps_impl()),
Review Comment:
Noticed that these were actual duplicated.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Implement ST_SimplifyPreserveTopology using geos [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #243:
URL: https://github.com/apache/sedona-db/pull/243#discussion_r2462948154
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -1399,3 +1399,49 @@ def test_st_isvalidreason(eng, geom, expected):
else:
query = f"SELECT ST_IsValidReason({geom_or_null(geom)})"
eng.assert_query_result(query, expected)
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "tolerance", "expected"),
+ [
+ # Basic linestring simplification
+ (
+ "LINESTRING (0 0, 0 10, 0 51, 50 20, 30 20, 7 32)",
+ 2,
+ "LINESTRING(0 0,0 51,50 20,30 20,7 32)",
+ ),
+ # Short linestring preserves endpoints
+ (
+ "LINESTRING (0 0, 0 10)",
+ 20,
+ "LINESTRING(0 0,0 10)",
+ ),
+ # Null handling
+ (None, 2, None),
+ (None, None, None),
+ ("LINESTRING (0 0, 0 10)", None, None),
+ # Empty geometries
+ ("LINESTRING EMPTY", 2, "LINESTRING EMPTY"),
+ ("POINT EMPTY", 2, "POINT EMPTY"),
+ ("POLYGON EMPTY", 2, "POLYGON EMPTY"),
+ # POLYGON with hole - topology preserved, both rings simplified
+ (
+ "POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0),(5 5, 5 6, 6 6, 8 5, 5 5))",
+ 20,
+ "POLYGON((0 0,10 0,10 10,0 10,0 0),(5 5,5 6,8 5,5 5))",
+ ),
+ # MULTIPOLYGON - both polygons preserved with topology preservation
+ (
+ "MULTIPOLYGON (((100 100, 100 130, 130 130, 130 100, 100 100)),((0 0, 10 0, 10 10, 0 10, 0 0),(5 5, 5 6, 6 6, 8 5, 5 5)))",
+ 20,
+ "MULTIPOLYGON(((100 100,100 130,130 130,130 100,100 100)),((0 0,10 0,10 10,0 10,0 0),(5 5,5 6,8 5,5 5)))",
+ ),
+ ],
+)
+def test_st_simplifypreservetopology(eng, geom, tolerance, expected):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ f"SELECT ST_AsText(ST_SimplifyPreserveTopology({geom_or_null(geom)}, {val_or_null(tolerance)}))",
Review Comment:
Noticed this inconsistency and thought that `ST_AsText` seems to normalize the test output. But I agree, it's better to keep all test cases aligned with similar approach.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Implement ST_SimplifyPreserveTopology using geos [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #243:
URL: https://github.com/apache/sedona-db/pull/243#discussion_r2463837677
##########
c/sedona-geos/src/st_simplifypreservetopology.rs:
##########
@@ -0,0 +1,203 @@
+// 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::error::Result;
+use datafusion_common::DataFusionError;
+use datafusion_expr::ColumnarValue;
+use geos::Geom;
+use sedona_expr::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;
+
+/// ST_SimplifyPreserveTopology() implementation using the geos crate
+pub fn st_simplify_preserve_topology_impl() -> ScalarKernelRef {
+ Arc::new(STSimplifyPreserveTopology {})
+}
+
+#[derive(Debug)]
+struct STSimplifyPreserveTopology {}
+
+impl SedonaScalarKernel for STSimplifyPreserveTopology {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()],
+ WKB_GEOMETRY,
+ );
+
+ matcher.match_args(args)
+ }
+
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ let tolerance: Option<f64>;
+ let arg1 = args[1].cast_to(&DataType::Float64, None)?;
+ if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
+ if scalar_arg.is_null() {
+ tolerance = None;
+ } else {
+ tolerance = Some(f64::try_from(scalar_arg.clone())?);
+ }
+ } else {
+ return Err(DataFusionError::Execution(format!(
+ "Invalid tolerance: {:?}",
+ args[1]
+ )));
+ }
Review Comment:
Thanks for pointing this out, nice that the array input was relatively easy to handle with ColumnarValue. Implementation is now updated with this change.
Btw. Does this introduce some room for performance/memory improvement? With this approach and using scalar value, we will copy it for all geometries, potentially quite many times.
Thinking if these tracks should be separated, or might be just micro optimization...
```rust
let executor = GeosExecutor::new(arg_types, args);
let tolerance_arg = args[1].cast_to(&DataType::Float64, None)?;
let mut builder = BinaryBuilder::with_capacity(
executor.num_iterations(),
WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
);
match tolerance_arg {
ColumnarValue::Scalar(scalar_val) => {
// FAST PATH: Extract scalar once, reuse for all geometries
...
}
ColumnarValue::Array(_) => {
// ARRAY PATH: Use iterator for per-row tolerances
...
}
}
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org