feat(c/sedona-geos): Implement ST_Simplify #295
Conversation
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
A few optional notes to consider...thank you for the thorough test cases!
|
|
||
| let geometry = match (initial_type, geometry.geometry_type()) { |
There was a problem hiding this comment.
| let geometry = match (initial_type, geometry.geometry_type()) { | |
| // GEOS simplifies MultiPolygon/MultiLineString to Polygon/Linestring; however, we | |
| // attempt to keep the original geometry type for PostGIS compatibility. | |
| let geometry = match (initial_type, geometry.geometry_type()) { |
There was a problem hiding this comment.
optional: It might be nice to explain why we need the wrapper!
|
|
||
| #[rstest] | ||
| fn udf(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType) { |
There was a problem hiding this comment.
Thank you for all of these test cases! Are there any sources we should acknowledge for them? (May be useful to update our own tests at some point if there are sources you consulted!)
There was a problem hiding this comment.
This is a lot of rust tests. FYI, for the future, you can be a little light on Rust tests, since they're kind of verbose. The python tests (which are very concise) is where we try to be comprehensive. e.g I'll often include a single empty geom in Rust tests, while in Python I'll test empty geoms for all types.
There's no need to delete these. Just mentioning it for the future because I know it can be kind of annoying to write so many of these.
There was a problem hiding this comment.
Thank you for all of these test cases! Are there any sources we should acknowledge for them? (May be useful to update our own tests at some point if there are sources you consulted!)
The implementation is based on the Spec defined in POSTGIS I run the test cases specified there manually on the docker container, then proceed to assert in code that the behavior is compliant.
Another place I consult for the test cases is Geos test suite ST_Simplify
There was a problem hiding this comment.
This is a lot of rust tests. FYI, for the future, you can be a little light on Rust tests, since they're kind of verbose. The python tests (which are very concise) is where we try to be comprehensive. e.g I'll often include a single empty geom in Rust tests, while in Python I'll test empty geoms for all types.
I find it easier to test in Rust, especially as it's strongly typed, for Instance, POSTGIS amd Sedona tend to handle EMPTY differently when testing with python, Sedona returns EMPTY while POSTGIS returns None, would require me including conditional checks in test function to get them to coexist happily, but Rust Serializes and Deserializes it Just fine.
But yeah, I'd be a little light on the Rust tests, and focus testing energy on Python's
There was a problem hiding this comment.
It's useful for us to have the Rust tests because we'd like to implement a faster version at some point, and when we have Rust code we'll need the Rust tests to cover the behaviour. (But yes, the minimum requirement for wrapping external library behaviour is a "is it plugged in" check at the Rust level).
3 participants
Having read both Issue #244 and PR #243 I'm quite interested in knowing or having the test cases that SedonaDB and PostGIS diverge, before I proceed to do a custom implementation, but at the moment, It seems to look fine on my end;
Everything is up for review.
closes #244