feat(rust/sedona-functions): Implement ST_GeomFromWKBUnchecked()
#533
Conversation
|
|
||
| # TODO: Test error case after error message check is supported in `assert_query_result` | ||
| # Using invalid WKB elsewhere may result in undefined behavior. | ||
| # eng.assert_query_result("SELECT ST_AsText(ST_GeomFromWKBUnchecked(0x01))", None) |
There was a problem hiding this comment.
Follow-up PR perhaps: I found it's not possible to assert error message if the query fails.
There was a problem hiding this comment.
I think sometimes we do:
with pytest.raises(lib.SedonaError, match="foofy"):
eng.execute_and_collect("SELECT ...")Would that work here? If not I think it is OK to skip this check for now.
| /// Documentation for `ST_GeomFromWKBUnchecked()`. | ||
| /// | ||
| /// Parameterized for reuse if `ST_GeogFromWKBUnchecked()` is implemented in the future. | ||
| fn doc_unchecked(name: &str, out_type_name: &str) -> Documentation { |
There was a problem hiding this comment.
Question: Is it the case that SQL reference (e.g. https://sedona.apache.org/sedonadb/latest/reference/sql/#st_geomfromwkb) can be auto-generated through these doc() traits, but we still have to update them manually?
There was a problem hiding this comment.
Yes, it's currently being updated manually. It is also in a state of flux right now where we're trying to separate the pages from each other in a way that each page can have a bit more detail, but still generate the main page. For now having the doc reference like you did here is perfect.
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you!
One suggestion on the Python test (either remove the todo or use pytest.raises() if that works) but other than that it looks great!
|
|
||
| # TODO: Test error case after error message check is supported in `assert_query_result` | ||
| # Using invalid WKB elsewhere may result in undefined behavior. | ||
| # eng.assert_query_result("SELECT ST_AsText(ST_GeomFromWKBUnchecked(0x01))", None) |
There was a problem hiding this comment.
I think sometimes we do:
with pytest.raises(lib.SedonaError, match="foofy"):
eng.execute_and_collect("SELECT ...")Would that work here? If not I think it is OK to skip this check for now.
| /// Documentation for `ST_GeomFromWKBUnchecked()`. | ||
| /// | ||
| /// Parameterized for reuse if `ST_GeogFromWKBUnchecked()` is implemented in the future. | ||
| fn doc_unchecked(name: &str, out_type_name: &str) -> Documentation { |
There was a problem hiding this comment.
Yes, it's currently being updated manually. It is also in a state of flux right now where we're trying to separate the pages from each other in a way that each page can have a bit more detail, but still generate the main page. For now having the doc reference like you did here is perfect.
|
Thank you for the review.
Yes it works, applied in 3524859 I was thinking wrapping such raises syntax with something like To make error assertion slightly more ergonomic, should we do this as follow up? |
|
CI python test failed in https://github.com/apache/sedona-db/actions/runs/21236733252/job/61106200223?pr=533 Looks like it's not related to the change, and I can't reproduce it locally with repeated test runs. Let me re-trigger CI to see if it is reproducible. |
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you!
I believe the CI failure is due to pandas 3.0 (released today).
2 participants
Rationale
ST_GeomFromWKBUnchecked()is equivalent toST_GeomFromWKB(), but it skips validation during conversion.This API is more efficient but potentially unsafe, so it is exposed as a separate function to make the lack of safety explicit.
An alternative would be to control validation via an additional argument, but this was not chosen for the above reason.
Implementation
The function kernel for
ST_GeomFromWKBUnchecked()is already implemented, this PR only exposed such functionality through a new function, and add tests.