feat: add ST_NumPoints alias for ST_NPoints#989
Conversation
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you!
We actually have an implementation of ST_NumPoints in c/sedona-geos that we should remove if we go ahead with this. I believe there are some tests in python/sedonadb/tests/functions that assert the behaviour of ST_NumPoints and STNPoints against PostGIS, whose documentation says they are aliases but we should make sure (or deliberately treat them as aliases anyway).
I think the alias behaviour is the one we want (BigQuery and possibly others treat them as exact aliases too).
ajaypadwal73
force-pushed
the
feat/st-numpoints-alias
branch
from
61e3b96 to
8d8db4c
Compare
ST_NumPoints is now an alias for ST_NPoints (registered via with_aliases in rust/sedona-functions). The separate GEOS implementation in c/sedona-geos was LineString-only (returned NULL for other types). Remove it to avoid the duplicate registration conflict. Update test_st_numpoints expected values to reflect alias behavior: ST_NumPoints now counts vertices across all geometry types, matching ST_NPoints and PostGIS documentation.
|
@paleolimbot Thanks for the context! Agreed - treating them as exact aliases (consistent with BigQuery) makes more sense than strict PostGIS LineString-only behavior. The old GEOS st_numpoints implementation has been removed and the Python tests updated to reflect the alias semantics. |
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you! The PostGIS docs indicate that these functions are supposed to be aliases but they don't seem to be (as you just found out). I think an alias is the right move.
One note on the test but other that this is good to go!
| ("geom", "expected"), | ||
| [ | ||
| (None, None), | ||
| ("POINT EMPTY", None), | ||
| ("POINT EMPTY", 0), | ||
| ("LINESTRING EMPTY", 0), | ||
| ("POLYGON EMPTY", None), | ||
| ("MULTIPOINT EMPTY", None), | ||
| ("MULTILINESTRING EMPTY", None), | ||
| ("MULTIPOLYGON EMPTY", None), |
There was a problem hiding this comment.
Sorry to bikeshed here, but can we just remove this test and add a second line to the ST_NPoints test? That will ensure that the behaviour stays in sync and any added parameters always apply to both.
There was a problem hiding this comment.
@paleolimbot Done - standalone test_st_numpoints removed and ST_NumPoints assertion added directly into test_st_points alongside ST_NPoints, sharing the same parametrized cases.
…ctations Per review feedback: remove standalone test_st_numpoints from test_functions.py and add ST_NumPoints as a third assertion in test_st_points, keeping both aliases in sync automatically. Also update test_geog_mechanical_transforms.py ST_NumPoints expected values to reflect alias behavior (all geometry types return counts, not just LineStrings).
PostGIS treats ST_NumPoints as LineString-only in practice, returning NULL for other geometry types, even though its documentation lists it as an alias for ST_NPoints. Skip the ST_NumPoints assertion on PostGIS to avoid false failures.
2 participants
Summary
ST_NumPointsas an alias forST_NPointsinst_points.rsCloses part of #200.
Changes
rust/sedona-functions/src/st_points.rs: added.with_aliases(vec!["st_numpoints".to_string()])tost_npoints_udf()and a corresponding alias test