feat(rust/sedona-functions): Implement ST_GeomFromEWKT#498
feat(rust/sedona-functions): Implement ST_GeomFromEWKT#498paleolimbot merged 15 commits intoapache:mainfrom
Conversation
paleolimbot
left a comment
There was a problem hiding this comment.
Only minor nits from me...this looks great!
I think I still don't understand how the item-level CRS should be handled
Me too! Supporting both type-level and item-level CRS is new territory and feel free to flag anything that doesn't make sense.
| // TODO | ||
| // engine: Option<Arc<dyn CrsEngine + Send + Sync>>, |
There was a problem hiding this comment.
Feel free to leave this out for now...the CRS engine variants of some of the other functions aren't currently used with an engine and we probably need a more reliable way to inject support for validation.
There was a problem hiding this comment.
Oh, thanks. Then, I remove this. I noticed engine is currently unused, but wondered if this is somehow useful for testing.
| let srid: u16 = match srid_str.parse() { | ||
| Ok(srid) => srid, | ||
| Err(_) => return None, | ||
| }; |
| DataType::Utf8View => arrow_array::create_array!( | ||
| Utf8View, | ||
| [ | ||
| Some("SRID=4326;POINT (1 2)"), | ||
| Some("SRID=3857;POINT (1 2)"), | ||
| None, | ||
| Some("POINT (3 4)"), | ||
| Some("SRID=0;POINT (5 6)") | ||
| ] | ||
| ) as ArrayRef, |
There was a problem hiding this comment.
I don't think you need the repeated case here (I'm pretty sure tester.invoke_array() will do that cast for you).
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you!
I opened #501 to track CRS validation. There are a number of places where the situation can and should be improved 🙂
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
|
I kept this pull request draft as I was wondering if I should add Python tests. However, since the output is different from PostGIS, probably it's not worth...? If I should add some, I can do. |
In this case you probably could check |
|
Thanks, I added some tests, but I found
|
| # TODO: currently, a per-item-CRS geometry cannot be cast to string | ||
| # eng.assert_query_result( | ||
| # f"SELECT ST_GeomFromEWKT({val_or_null(ewkt)})", | ||
| # expected, | ||
| # ) |
There was a problem hiding this comment.
If you rebase or merge in the main branch I think this will work:
| # TODO: currently, a per-item-CRS geometry cannot be cast to string | |
| # eng.assert_query_result( | |
| # f"SELECT ST_GeomFromEWKT({val_or_null(ewkt)})", | |
| # expected, | |
| # ) | |
| eng.assert_query_result( | |
| f"SELECT ST_AsWKT(ST_GeomFromEWKT({val_or_null(ewkt)}))", | |
| expected, | |
| ) |
Also feel free to remove this bit and I can follow up when I do ST_GeomFromEKWB() (FWIW I prefer not to merge TODOs that don't have an tracker link associated with them, since TODOs in the code base are much harder to prioritize than issues.)
There was a problem hiding this comment.
Fair enough! Then, I remove this for now as ST_AsWKT() doesn't work either.
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
|
Sorry, I just didn't notice |
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
Now that we have item-level CRS, I think
ST_GeomFromEWKTcan be implemented. UnlikeST_GeomFromEWKB(#490), if I understand correctly, EWKT doesn't need a dedicated parser except for splitingSRID=...;and pass the rest to the existing WKT parser.It seems to work. I think I still don't understand how the item-level CRS should be handled, so this pull request takes some more time to finish.