-
Notifications
You must be signed in to change notification settings - Fork 30
feat(rust/sedona-functions): Add ST_Translate() #267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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 implements the ST_Translate() function in Rust, which translates geometry coordinates by fixed X and Y offsets. The implementation leverages the existing CrsTransform infrastructure and includes comprehensive test coverage and benchmarking.
Key changes:
- New
ST_Translate()function implementation with support for 2D and 3D geometries - Integration into the function registry with alphabetically sorted entries
- Test and benchmark coverage for the new functionality
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rust/sedona-functions/src/st_translate.rs | Core implementation of ST_Translate function with geometry transformation logic |
| rust/sedona-functions/src/register.rs | Alphabetically reorganized function registry and added st_translate_udf registration |
| rust/sedona-functions/src/lib.rs | Added st_translate module declaration |
| rust/sedona-functions/benches/native-functions.rs | Added benchmark tests for ST_Translate with Point and LineString geometries |
| python/sedonadb/tests/functions/test_transforms.py | Added parametrized tests for ST_Translate covering edge cases and geometry types |
| benchmarks/test_transform.py | Added performance benchmark comparing SedonaDB and DuckDB implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
petern48
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just suggested some optional nits about testing.
| ("POINT Z EMPTY", 1.0, 2.0, "POINT Z (nan nan nan)"), | ||
| ("POINT (0 1)", 1.0, 2.0, "POINT (1 3)"), | ||
| ("POINT Z (0 1 2)", 1.0, 2.0, "POINT Z (1 3 2)"), | ||
| ("LINESTRING EMPTY", 1.0, 2.0, "LINESTRING EMPTY"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: A few cases that aren't tested anywhere (in python or rust). I don't think any would fail, but I think some would be nice to test. Particularly non-point geoms, so it's clear it works on them too. Personally, I often look at tests to see how I can use a function when documentation isn't specific enough.
- geometry arg
- non-empty geometries other than POINT (e.g polygon, multi-*)
- any
Mdimension
- dx/dy arg
- zero or negative
- integers (inspired by this bug: Casts ints to floats in st_buffer and st_dwithin args #32)
Co-authored-by: Peter Nguyen <petern0408@gmail.com>
This PR implements
ST_Translate().