You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "james-willis (via GitHub)" <gi...@apache.org> on 2026/08/26 20:38:33 UTC
[PR] fix: require a spatial item in item_crs input matchers [sedona-db]
james-willis opened a new pull request, #1194:
URL: https://github.com/apache/sedona-db/pull/1194
### The bug
`SedonaType::is_item_crs()` answers a shape question: is this an Arrow struct with exactly two fields named `item` and `crs`. It never checks the type of `item`, and `ArgMatcher::is_item_crs()` delegates straight to it.
That is the right question for a *return* type — "did this kernel produce an item_crs type" — but the wrong one for an *input* matcher. Four functions register an item_crs kernel with the bare matcher, so any two-field `(item, crs)` struct resolved to them regardless of what `item` held:
```sql
SELECT ST_SRID(named_struct('item', 1, 'crs', arrow_cast('EPSG:4326', 'Utf8View')));
```
### Measured before/after
Run against a `SedonaContext` on the base branch, then again with this change:
| Function | Kernel | Before | After |
|---|---|---|---|
| `ST_SRID` | `StSridItemCrs`, `st_srid.rs:109` | returns `4326` — silently wrong, no error | plan error: `st_srid(struct): No kernel matching arguments` |
| `ST_CRS` | `StCrsItemCrs`, `st_srid.rs:214` | returns `'EPSG:4326'` — silently wrong, no error | plan error: `st_crs(struct): No kernel matching arguments` |
| `ST_AsEWKB` | `STAsEWKBItemCrs`, `st_asewkb.rs:105` | exec error: `SedonaDB internal error: Can't iterate over Arrow(Int64) as Wkb` | plan error: `st_asewkb(struct): No kernel matching arguments` |
| `ST_GeoHash` | `STGeoHashItemCrs`, `st_geohash.rs:72` and `:78` | exec error: `SedonaDB internal error: Expected geometry or geography argument but got Arrow(Int64)` | plan error: `st_geohash(struct): No kernel matching arguments` |
`ST_SRID` and `ST_CRS` are the severe half. The other two at least fail — noisily and with a message that reads as an engine bug rather than a bad query, but they fail. `ST_SRID` and `ST_CRS` hand back a confident answer about the CRS of an integer, which nothing downstream has any reason to distrust.
### The fix
A new shared matcher in `rust/sedona-schema/src/matchers.rs`:
```rust
pub fn is_item_crs_of(inner: Arc<dyn TypeMatcher + Send + Sync>) -> Arc<dyn TypeMatcher + Send + Sync>
```
It asks both halves: the argument is an item_crs struct **and** its wrapped item matches `inner`. The item type comes from `SedonaType::from_storage_field()` on `fields[0]`, so an item carrying extension metadata is interpreted the same way it would be anywhere else. A field this crate cannot interpret does not match rather than erroring — matching is a predicate, and another kernel may still apply to the argument.
`is_item_crs()` stays. `ST_SetSRID()` and `item_crs.rs` legitimately need the shape-only question; its doc comment now says which of the two to reach for.
### Why `is_geometry_or_geography()` for all four
I read what each kernel does with the item rather than assuming, and the same matcher is correct in all four cases — because in every one of them the item matcher is just the sibling kernel's own predicate applied one level in:
- **`ST_SRID` / `ST_CRS`** never touch the item's bytes; they read the `crs` column and the item's null buffer. So no narrower matcher is *forced* by the implementation — but the sibling kernels `StSrid`/`StCrs`, which handle the plain types, both declare `is_geometry_or_geography()`. The item_crs form of a function should accept exactly what its plain form accepts; anything else would mean `ST_SRID` answers for a row-level CRS on a type it refuses at the column level.
- **`ST_AsEWKB`** iterates the item as WKB (`execute_wkb_void`) and writes EWKB. Its sibling `STAsEWKB` declares `is_geometry_or_geography()`, and EWKB output is not restricted to planar edges, so `is_geometry()` would be wrong.
- **`ST_GeoHash`** unwraps the item and runs the same geometry path as `STGeoHash`, which resolves a planar *or* spherical bounder from the session runtime. Both siblings declare `is_geometry_or_geography()`; narrowing to `is_geometry()` would reject `WKB_GEOGRAPHY_ITEM_CRS`, which `udf_geography` already covers as a supported case.
`is_item_crs_of` takes the inner matcher as a parameter precisely so a future kernel with a narrower item requirement can say so, and there is a unit test pinning that `is_item_crs_of(is_geometry())` and `is_item_crs_of(is_geography())` discriminate correctly.
### What was deliberately left alone
- **`ST_SetSRID`** (`st_setsrid.rs:215`) uses `is_item_crs()` on the *return* type, not as an input matcher. Shape is exactly the question it is asking there.
- **Every `ItemCrsKernel::wrap_impl`-wrapped function** (`ST_Area`, `ST_GeometryType`, …) is already safe. `return_type_handle_item_crs` in `rust/sedona-expr/src/item_crs.rs` strips the item out and delegates to the inner kernel's own matcher, which rejects a non-spatial item on its own. `item_crs.rs:403` and `item_crs.rs:188` keep using `is_item_crs()` for the same reason: they are asking "is any argument an item_crs type at all" before unwrapping.
The `crs` field's *type* is still unchecked by both matchers — that is a separate gap and out of scope here.
### Tests
- `matchers.rs`: three unit tests on the new matcher — that it accepts the real item_crs types and rejects an item_crs-shaped struct wrapping `Int64` (pinning that `is_item_crs()` still accepts it, i.e. the two matchers differ deliberately); that a narrower inner matcher discriminates geometry from geography; that an uninterpretable item field declines instead of erroring.
- One regression test per affected function asserting every kernel of the UDF returns `Ok(None)` for a non-spatial item_crs argument, that the UDF as a whole then fails resolution with "No kernel matching arguments", and that a real `WKB_GEOMETRY_ITEM_CRS` argument still resolves. `ST_GeoHash` covers both arities.
I verified all three regression tests fail on the base branch (`Some(Arrow(UInt32))` / `Some(Arrow(Binary))` / `Some(Arrow(Utf8))` where `None` is expected) before they pass with the fix.
```
$ cargo test -p sedona-schema -p sedona-functions -p sedona-expr
test result: ok. 100 passed; 0 failed (sedona-expr)
test result: ok. 550 passed; 0 failed (sedona-functions)
test result: ok. 65 passed; 0 failed (sedona-schema)
$ cargo fmt --all -- --check # clean
$ cargo clippy -p sedona-schema -p sedona-functions --all-targets
# clean; the only warning is the pre-existing
# `unknown lint: clippy::chunks_exact_to_as_chunks` in sedona-testing, from main
```
No Python integration tests were added — the Rust tests cover the planning behaviour that changed, and the SQL repro above was verified directly against `SedonaContext`.
### Stacking
> **This PR is stacked on #1163 and must merge after it.**
Two of the five call sites are `ST_GeoHash` kernels that only exist on `james/st-geohash`, so this branch is based on that branch rather than on `main`. I would have targeted `james/st-geohash` directly, but that branch lives on my fork and GitHub only accepts a base branch that exists in this repo — so this targets `main` and therefore currently shows #1163's commits as well. Once #1163 merges, the diff here reduces to the two commits that are actually mine:
- `fix(rust/sedona-schema): add ArgMatcher::is_item_crs_of()`
- `fix(rust/sedona-functions): reject a non-spatial item_crs argument`
Reviewing just those two is enough; everything below them is #1163.
--
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] fix: require a spatial item in item_crs input matchers [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on PR #1194:
URL: https://github.com/apache/sedona-db/pull/1194#issuecomment-5445922344
converted to draft since this is stacked on ST_Geohash.
--
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] fix: require a spatial item in item_crs input matchers [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #1194:
URL: https://github.com/apache/sedona-db/pull/1194#discussion_r3876334879
##########
rust/sedona-schema/src/matchers.rs:
##########
@@ -383,6 +402,32 @@ impl TypeMatcher for IsItemCrs {
}
}
+#[derive(Debug)]
+struct IsItemCrsOf {
+ item_matcher: Arc<dyn TypeMatcher + Send + Sync>,
+}
+
+impl TypeMatcher for IsItemCrsOf {
+ fn match_type(&self, arg: &SedonaType) -> bool {
+ if !arg.is_item_crs() {
Review Comment:
done
--
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] fix: require a spatial item in item_crs input matchers [sedona-db]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #1194:
URL: https://github.com/apache/sedona-db/pull/1194#discussion_r3869842731
##########
rust/sedona-schema/src/matchers.rs:
##########
@@ -383,6 +402,32 @@ impl TypeMatcher for IsItemCrs {
}
}
+#[derive(Debug)]
+struct IsItemCrsOf {
+ item_matcher: Arc<dyn TypeMatcher + Send + Sync>,
+}
+
+impl TypeMatcher for IsItemCrsOf {
+ fn match_type(&self, arg: &SedonaType) -> bool {
+ if !arg.is_item_crs() {
Review Comment:
[P2] Could we validate the `crs` field as part of this matcher too? `is_item_crs()` only checks field names, so `struct<item: geoarrow.wkb/binary, crs: Utf8>` passes here even though all four kernels assume `Utf8View`. I reproduced this from a Parquet column with that schema: `ST_SRID`, `ST_AsEWKB`, and `ST_GeoHash` fail with internal StringView downcast errors, while `ST_CRS` hits the DataFusion assertion that it returned `Utf8` after promising `Utf8View`. I know the PR description calls this a separate gap, but it looks like the same false-positive class at the new input-matcher boundary. Maybe `SedonaType::is_item_crs()` should also require `fields[1].data_type() == DataType::Utf8View`, with `Utf8` and `Int64` CRS-field cases.
--
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] fix: require a spatial item in item_crs input matchers [sedona-db]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #1194:
URL: https://github.com/apache/sedona-db/pull/1194#discussion_r3869842731
##########
rust/sedona-schema/src/matchers.rs:
##########
@@ -383,6 +402,32 @@ impl TypeMatcher for IsItemCrs {
}
}
+#[derive(Debug)]
+struct IsItemCrsOf {
+ item_matcher: Arc<dyn TypeMatcher + Send + Sync>,
+}
+
+impl TypeMatcher for IsItemCrsOf {
+ fn match_type(&self, arg: &SedonaType) -> bool {
+ if !arg.is_item_crs() {
Review Comment:
Would it be worth checking the `crs` field here too? `is_item_crs()` only checks the field names, so a struct with a spatial `item` and `crs: Utf8` still gets through even though these kernels read it as `Utf8View`. I tried that with a Parquet column and got an internal downcast error; `ST_CRS` also hit a return-type mismatch. Maybe `is_item_crs()` could require `Utf8View` for the second field?
--
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] fix: require a spatial item in item_crs input matchers [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis merged PR #1194:
URL: https://github.com/apache/sedona-db/pull/1194
--
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