refactor(rust/sedona-raster-functions): extract CachedCrsToSRIDMapping and simplify SRID/CRS logic#590
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors CRS/SRID handling to reuse a shared CRS→SRID cache and simplify raster SRID/CRS UDF logic, while also exposing a public/global PROJ engine helper and adding tests for missing/unsupported CRS cases.
Changes:
- Added
CachedCrsToSRIDMappinginsedona-schema::crsand updated SRID UDFs to use it. - Simplified
RS_SRID/RS_CRSraster UDF logic (early returns) and added raster CRS string ref accessor. - Made
with_global_proj_enginepublic + generic and added tests for missing CRS / CRS-without-SRID.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/sedona-schema/src/crs.rs | Introduces reusable cached CRS→SRID mapping + tweaks CRS deserialization behavior. |
| rust/sedona-raster/src/array.rs | Refactors band metadata access and adds RasterRefImpl::crs_str_ref(). |
| rust/sedona-raster-functions/src/rs_srid.rs | Updates RS_SRID/RS_CRS to use caching and early-return flow; adds tests. |
| rust/sedona-functions/src/st_srid.rs | Replaces per-batch HashMap caching with shared CachedCrsToSRIDMapping. |
| c/sedona-proj/src/st_transform.rs | Makes global PROJ engine helper public and returns generic Result<R>. |
| c/sedona-proj/src/lib.rs | Exposes st_transform module publicly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
65a552d to
cd4a87e
Compare
…S logic Add CachedCrsToSRIDMapping to sedona-schema for reusable CRS-to-SRID caching. Refactor RS_SRID, RS_CRS, and ST_SRID to use it, replacing deeply nested match arms with early-return patterns. Add crs_str_ref() to RasterRefImpl and make with_global_proj_engine generic and public.
fa7e695 to
0de5bb2
Compare
c/sedona-proj/src/lib.rs
Outdated
| pub mod register; | ||
| pub mod sd_order_lnglat; | ||
| mod st_transform; | ||
| pub mod st_transform; |
There was a problem hiding this comment.
Can this be reverted? (I don't see a reference to with_global_proj_engine() in this PR?)
We should probably move the PROJ engine to its own public module and/or abstract it so we can switch to something faster for simple transforms.
There was a problem hiding this comment.
Good catch. We may need to make it pub later, but definitely not in this PR. I'll revert it.
c/sedona-proj/src/st_transform.rs
Outdated
| thread_local! { | ||
| static PROJ_CACHE_SIZE: OnceCell<usize> = const { OnceCell::new() }; | ||
| } |
There was a problem hiding this comment.
No. I'll remove it.
| /// Translating CRS into integer SRID with a cache to avoid expensive CRS deserialization. | ||
| pub struct CachedCrsToSRIDMapping { | ||
| cache: HashMap<Cow<'static, str>, u32>, | ||
| } |
There was a problem hiding this comment.
Not for this PR, but the abbreviated CRS should probably have this too (I have a batch-local cache for st_setsrid):
sedona-db/rust/sedona-functions/src/st_setsrid.rs
Lines 510 to 542 in cb713ef
There was a problem hiding this comment.
I'll handle it when implementing RS_SetSRID and RS_SetCRS.
Summary
CachedCrsToSRIDMappingstruct intosedona-schema::crsfor reusable CRS-to-SRID caching, replacing ad-hocHashMapcaching inST_SRIDand duplicated logic inRS_SRIDRS_SRIDandRS_CRSinsedona-raster-functionsto use early-return patterns instead of deeply nested match armscrs_str_ref()toRasterRefImplfor zero-copy CRS string accesswith_global_proj_enginegeneric (pub fn ... -> Result<R>) and public for external useDependencies
pr2-better-band-metadata-ref)