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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
