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)
+
+
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("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)
+
+
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
