petern48 commented on code in PR #409:
URL: https://github.com/apache/sedona-db/pull/409#discussion_r2611162226
##########
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:
When it comes to geospatial knowledge. You just learn overtime that there's
four possible dimensions. X and Y (required), and Z and M (both optional).
These cases always existed, but they were less interesting cases for your
previous functions which only returned counts of rings. Whether an M or Z
dimension existed or not is unlikely to fail for those "counting" functions. We
could totally add them, but they were less interesting or unlikely to break
things.
For this ST_ExteriorRing, it doesn't just count rings, it **returns them**.
Specifically, the returned geometry should also have Z and M dimensions if
their inputs did. It's certainly possible for underlying implementations to
forget to implement Z and M support. In that case, the function might right
`LINESTRING` except missing the Z and M dimensions. That's why Z and M tests
are more interesting for this function, hence we want to test them and make
sure it doesn't test the wrong input.
In contrast, if ST_NumRings completely disregarded Z and M dimensions, it
probably would still return the right count, since it doesn't really care about
the exact contents of the rings, just the number of them. This intuition isn't
quite something contributors would be expected to reason about, so don't worry
too much if you can't follow the logic.
--
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]
