Skip to content

Comments

Add ST_SetCrs#37

Merged
jiayuasu merged 4 commits intoapache:mainfrom
jesspav:st_setcrs
Sep 8, 2025
Merged

Add ST_SetCrs#37
jiayuasu merged 4 commits intoapache:mainfrom
jesspav:st_setcrs

Conversation

@jesspav
Copy link
Collaborator

@jesspav jesspav commented Sep 8, 2025

Summary

Currently, ST_SetSRID is out of line with other databases as it expects and accepts a string to allow for CRS codes and proj json. This change splits it into ST_SetSrid (which complies with standard expectations for that function) and the new ST_SetCRS which allows our users to provide more interesting and complex CRS definitions.

Issue: #29

Testing

Added unit tests, updated integration tests and ran tests with cargo run. benchmark tests will be provided in another PR.

Example

Examples correctly identifying setting the CRS:

> select ST_SRID(ST_SetSrid(ST_GeomFromText('POINT (1 1)'), 4837)) as srid;
┌────────┐
│  srid  │
│ uint32 │
╞════════╡
│   4837 │
└────────┘

> select st_srid(ST_SetCrs(ST_GeomFromText('POINT (1 1)'), 'EPSG:4837')) as geo;
┌────────┐
│   geo  │
│ uint32 │
╞════════╡
│   4837 │
└────────┘

Examples not accepting the other's arguments:

> select ST_SetSrid(ST_GeomFromText('POINT (1 1)'), 'EPSG:4837') as geo;
This feature is not implemented: st_setsrid([Wkb(Planar, None), Arrow(Utf8)]): No kernel matching arguments

> select ST_SetCrs(ST_GeomFromText('POINT (1 1)'), 4837) as geo;
This feature is not implemented: st_setcrs([Wkb(Planar, None), Arrow(Int64)]): No kernel matching arguments

@jesspav jesspav requested a review from Copilot September 8, 2025 16:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR splits the existing ST_SetSRID function to better align with database standards by creating two separate functions: ST_SetSRID which now accepts only integer SRID codes, and a new ST_SetCRS function which accepts string-based CRS definitions including EPSG codes and proj JSON.

  • Refactored ST_SetSRID to accept only numeric SRID inputs for standards compliance
  • Added new ST_SetCRS function that accepts string-based CRS identifiers
  • Extracted common logic into a shared determine_return_type function

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
rust/sedona-functions/src/st_setsrid.rs Implements the function split with new ST_SetCRS struct, refactored validation logic, and comprehensive test coverage
rust/sedona-functions/src/register.rs Registers the new st_set_crs_udf function in the function set and exports it in the stubs module

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@jesspav jesspav marked this pull request as ready for review September 8, 2025 17:43
@jesspav jesspav requested a review from paleolimbot September 8, 2025 18:10
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@jiayuasu jiayuasu merged commit 9657a5e into apache:main Sep 8, 2025
5 checks passed
@jesspav jesspav deleted the st_setcrs branch November 14, 2025 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants