fix: Use topological (recursive) emptiness#80
Conversation
| // Recursive emptiness (PostGIS / JTS topological semantics). | ||
| // A collection whose every leaf is empty is itself reported as empty, | ||
| // even though it has structurally non-zero children. | ||
| // | ||
| // NB: GeoArrow explicitly denies recursive collections, | ||
| // so we only need to test one level. |
There was a problem hiding this comment.
It sounds like this only applies to GeometryCollection? Can any other supported geometry types have an empty child?
There was a problem hiding this comment.
Good question... so in OGC terms, no, it actually applies to everything. MultiPoints can have a mix of empty and non-empty points, etc... My quick reading of the GeoArrow spec also seems to allow this. However, when writing up some extra tests for those, I discovered that the WKT parser we depend on only accepts mixed empty/non-empty multi-linestrings (georust/wkt#111 seems to acknowledge this, but the symptom I see right now isn't a crash but a parse error). And multipolygons mixing empty and non-empty seem to crash, but those should also technically be allowed.
There was a problem hiding this comment.
You could have a few WKB fixtures for this (this is what we do in SedonaDB). geoarrow.pyarrow's WKT parser should be able to generate these for you.
There was a problem hiding this comment.
Good call! I generated them with PostGIS ST_AsBinary and interpolated the literal, but same idea.
The multipolygon case I came up with is still broken due to an assumption in geoarrow-array. I'm not sure if there is a geoarrow rule forbidding that structure, but it works in PostGIS 🤷
fbcf8a5 to
58583eb
Compare
3 participants
I stumbled upon some other edge cases when checking compatibility against PostGIS. Basically the PG stance (also verified in JTS, so I assume GEOS is also the same) is that emptiness checks should be recursive based on the principle of "does this thing actually represent at least a point or does the structure simply collapse to nothing."
This showed up as I was working on an implementation for
ST_Dump(PR soon :D) and trying to keep it native. After a bunch of test cases, I realized that PostGIS actually drops these, and there went the afternoon researching what we should do 😂