You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "2010YOUY01 (via GitHub)" <gi...@apache.org> on 2026/01/21 10:41:01 UTC
[PR] feat(rust/sedona-functions): Implement `ST_GeomFromWKBUnchecked()` [sedona-db]
2010YOUY01 opened a new pull request, #533:
URL: https://github.com/apache/sedona-db/pull/533
`ST_GeomFromWKBUnchecked()` is equivalent to `ST_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.
```sql
select ST_GeomFromWKB('0x01....', false); -- Skip validation via `false` arg
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-functions): Implement `ST_GeomFromWKBUnchecked()` [sedona-db]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #533:
URL: https://github.com/apache/sedona-db/pull/533#discussion_r2711991240
##########
rust/sedona-functions/src/st_geomfromwkb.rs:
##########
@@ -77,6 +92,29 @@ fn doc(name: &str, out_type_name: &str) -> Documentation {
.build()
}
+/// 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 {
Review 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?
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -1668,6 +1669,49 @@ def test_st_geomfromwkb(eng, geom):
eng.assert_query_result(f"SELECT ST_GeomFromWKB({wkb})", expected)
+# `ST_GeomFromWKBUnchecked` is not available in PostGIS
+@pytest.mark.parametrize("eng", [SedonaDB])
+@pytest.mark.parametrize(
+ ("geom"),
+ [
+ "POINT (1 1)",
+ "POINT EMPTY",
+ "LINESTRING EMPTY",
+ "POLYGON EMPTY",
+ "GEOMETRYCOLLECTION EMPTY",
+ "POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0))",
+ "MULTILINESTRING ((0 0, 1 1), (1 1, 2 2))",
+ "GEOMETRYCOLLECTION (POINT (0 0), LINESTRING (0 0, 1 1), POLYGON ((0 0, 0 1, 1 1, 1 0, 0 0)))",
+ ],
+)
+def test_st_geomfromwkbunchecked(eng, geom):
+ eng = eng.create_or_skip()
+
+ expected = geom
+ if geom == "POINT EMPTY":
+ # arrow-c returns POINT (nan nan) instead of POINT EMPTY
+ expected = "POINT (nan nan)"
+
+ wkb = shapely.from_wkt(geom).wkb
+ wkb = "0x" + wkb.hex()
+
+ eng.assert_query_result(f"SELECT ST_GeomFromWKBUnchecked({wkb})", expected)
+
+
+@pytest.mark.parametrize("eng", [SedonaDB])
+def test_st_geomfromwkbunchecked_invalid_wkb(eng):
+ eng = eng.create_or_skip()
+
+ # Invalid WKB payload can still convert to geometry column
+ eng.assert_query_result(
+ "SELECT ST_AsBinary(ST_GeomFromWKBUnchecked(0x01))", b"\x01"
+ )
+
+ # 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)
Review Comment:
Follow-up PR perhaps: I found it's not possible to assert error message if the query fails.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-functions): Implement `ST_GeomFromWKBUnchecked()` [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #533:
URL: https://github.com/apache/sedona-db/pull/533
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-functions): Implement `ST_GeomFromWKBUnchecked()` [sedona-db]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #533:
URL: https://github.com/apache/sedona-db/pull/533#issuecomment-3782521394
Thank you for the review.
> I think sometimes we do:
>
> ```python
> 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.
Yes it works, applied in [3524859](https://github.com/apache/sedona-db/pull/533/commits/35248596225206e2bae4ebaa25d53e1ee2e81f75)
I was thinking wrapping such raises syntax with something like
```
eng.assert_query_error("select foo()", Exception, "Function did not exist")
```
To make error assertion slightly more ergonomic, should we do this as follow up?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-functions): Implement `ST_GeomFromWKBUnchecked()` [sedona-db]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 closed pull request #533: feat(rust/sedona-functions): Implement `ST_GeomFromWKBUnchecked()`
URL: https://github.com/apache/sedona-db/pull/533
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-functions): Implement `ST_GeomFromWKBUnchecked()` [sedona-db]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #533:
URL: https://github.com/apache/sedona-db/pull/533#issuecomment-3782577086
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-functions): Implement `ST_GeomFromWKBUnchecked()` [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #533:
URL: https://github.com/apache/sedona-db/pull/533#discussion_r2713372737
##########
rust/sedona-functions/src/st_geomfromwkb.rs:
##########
@@ -77,6 +92,29 @@ fn doc(name: &str, out_type_name: &str) -> Documentation {
.build()
}
+/// 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 {
Review 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.
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -1668,6 +1669,49 @@ def test_st_geomfromwkb(eng, geom):
eng.assert_query_result(f"SELECT ST_GeomFromWKB({wkb})", expected)
+# `ST_GeomFromWKBUnchecked` is not available in PostGIS
+@pytest.mark.parametrize("eng", [SedonaDB])
+@pytest.mark.parametrize(
+ ("geom"),
+ [
+ "POINT (1 1)",
+ "POINT EMPTY",
+ "LINESTRING EMPTY",
+ "POLYGON EMPTY",
+ "GEOMETRYCOLLECTION EMPTY",
+ "POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0))",
+ "MULTILINESTRING ((0 0, 1 1), (1 1, 2 2))",
+ "GEOMETRYCOLLECTION (POINT (0 0), LINESTRING (0 0, 1 1), POLYGON ((0 0, 0 1, 1 1, 1 0, 0 0)))",
+ ],
+)
+def test_st_geomfromwkbunchecked(eng, geom):
+ eng = eng.create_or_skip()
+
+ expected = geom
+ if geom == "POINT EMPTY":
+ # arrow-c returns POINT (nan nan) instead of POINT EMPTY
+ expected = "POINT (nan nan)"
+
+ wkb = shapely.from_wkt(geom).wkb
+ wkb = "0x" + wkb.hex()
+
+ eng.assert_query_result(f"SELECT ST_GeomFromWKBUnchecked({wkb})", expected)
+
+
+@pytest.mark.parametrize("eng", [SedonaDB])
+def test_st_geomfromwkbunchecked_invalid_wkb(eng):
+ eng = eng.create_or_skip()
+
+ # Invalid WKB payload can still convert to geometry column
+ eng.assert_query_result(
+ "SELECT ST_AsBinary(ST_GeomFromWKBUnchecked(0x01))", b"\x01"
+ )
+
+ # 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)
Review Comment:
I think sometimes we do:
```python
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org