feat(c/sedona-proj): Implement item crs support for ST_Transform#531
feat(c/sedona-proj): Implement item crs support for ST_Transform#531paleolimbot merged 13 commits intoapache:mainfrom
Conversation
9570f39 to
3c13875
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements item-level CRS (Coordinate Reference System) support for the ST_Transform function, enabling transformations where the target CRS varies per row rather than being fixed at the type level.
Changes:
- Adds support for item-level CRS input/output in
ST_Transform, allowing per-row CRS specifications - Refactors argument handling to support both scalar and array CRS inputs with appropriate return type determination
- Optimizes transformation by caching when both source and target CRS are constant across all rows
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| rust/sedona-testing/src/testers.rs | Updates invoke method to compute return types with scalar arguments |
| rust/sedona-functions/src/executor.rs | Adds support for iterating over item-crs struct arrays by extracting geometry from the "item" field |
| c/sedona-proj/src/st_transform.rs | Major refactor to support item-crs inputs/outputs with new argument pattern matching and transformation logic |
| c/sedona-proj/Cargo.toml | Adds sedona-common dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| match maybe_wkb { | ||
| Some(wkb) => { | ||
| invoke_scalar(&wkb, crs_transform.as_ref(), &mut builder)?; | ||
| builder.append_value([]); |
There was a problem hiding this comment.
Just wanted to confirm the builder.append_value([]); is expected to be called after the invoke_scalar call.
There was a problem hiding this comment.
Yes, that is expected (invoke_scalar writes into the builder directly but the append_value is needed to force a new element from the builder).
| executor.execute_wkb_void(|maybe_wkb| { | ||
| match (maybe_wkb, crs_to_crs_iter.next().unwrap()) { | ||
| (Some(wkb), (Some(from_crs_str), Some(to_crs_str))) => { | ||
| let maybe_from_crs = deserialize_crs(from_crs_str)?; |
There was a problem hiding this comment.
Maybe caching could help here since this is per-row calculation?
There was a problem hiding this comment.
We have a thread local cache behind deserialize_crs() for this reason!
...there are probably more ways we could speed up per-row CRS operations once they actually work, though.
This PR integrates Item Crs inputs and outputs into ST_Transform, which now has a rather complicated signature with respect to what can happen:
There's probably no great way to implement all of that cleanly but I did try to reduce repetition as much as possible.