-
Notifications
You must be signed in to change notification settings - Fork 40
feat(rust/sedona-functions): Implement ST_Affine(), ST_Rotate(), and ST_Scale() #504
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
04322a3 to
427566d
Compare
|
I was hoping to add the variants with |
paleolimbot
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.
Thank you! This has to be the most arguments I have ever seen in a SQL function and managing all of them is a pain that you've nicely abstracted here.
using the glam crate.
I think this is OK. We sort of have this for 2D already but I'm not sure that will simplify this PR given the 3D support. If we do inline this we should do it in sedona-geometry and add the appropriate tests.
I was hoping to add the variants with origin of rotation or scaling, but, since this pull request is already a bit lengthy, I think it's probably better to address in a separate pull request.
Great!
| let executor = WkbExecutor::new(arg_types, args); | ||
| let mut builder = BinaryBuilder::with_capacity( | ||
| executor.num_iterations(), | ||
| WKB_MIN_PROBABLE_BYTES * executor.num_iterations(), |
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.
No need to do this here because I think we need a helper for it to be compact, but in this particular case we know the output will have the exact number of bytes as the input)
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.
Ah, true. Thanks for catching.
| match geom.as_type() { | ||
| geo_traits::GeometryType::Point(pt) => { | ||
| if pt.coord().is_some() { | ||
| write_wkb_point_header(writer, dims) | ||
| .map_err(|e| DataFusionError::Execution(e.to_string()))?; | ||
| write_transformed_coord(writer, pt.coord().unwrap(), mat, dim)?; | ||
| } else { | ||
| write_wkb_empty_point(writer, dims) | ||
| .map_err(|e| DataFusionError::Execution(e.to_string()))?; | ||
| } | ||
| } | ||
|
|
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.
If this can be written as a CrsTransform we might be able re-use some existing infrastructure:
sedona-db/rust/sedona-functions/src/st_translate.rs
Lines 123 to 135 in 9bf02f4
| #[derive(Debug)] | |
| struct Translate { | |
| deltax: f64, | |
| deltay: f64, | |
| } | |
| impl CrsTransform for Translate { | |
| fn transform_coord(&self, coord: &mut (f64, f64)) -> Result<(), SedonaGeometryError> { | |
| coord.0 += self.deltax; | |
| coord.1 += self.deltay; | |
| Ok(()) | |
| } | |
| } |
sedona-db/rust/sedona-functions/src/st_translate.rs
Lines 106 to 108 in 9bf02f4
| let trans = Translate { deltax, deltay }; | |
| transform(wkb, &trans, &mut builder) | |
| .map_err(|e| DataFusionError::External(Box::new(e)))?; |
https://github.com/apache/sedona-db/blob/main/rust/sedona-geometry/src/transform.rs#L259-L263
There's a chance what you have here is faster, though (no dyn like our current transform()), and we haven't implemented non-XY support for the CrsTransform yet ( #47 ).
No need to take on any of that here (I'll just add a note to that issue that we can update this section when we do get there).
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.
Sounds good to me. I'll try.
(As translation can also be handled by affine transformation, I was wondering if ST_Transform can also be implemented using glam. But, I guess there will be not much difference)
| fn write_transformed_coords<I>( | ||
| writer: &mut impl Write, | ||
| coords: I, | ||
| affine: &DAffine, | ||
| dim: &geo_traits::Dimensions, | ||
| ) -> Result<()> | ||
| where | ||
| I: DoubleEndedIterator, | ||
| I::Item: CoordTrait<T = f64>, | ||
| { | ||
| coords | ||
| .into_iter() | ||
| .try_for_each(|coord| write_transformed_coord(writer, coord, affine, dim)) | ||
| } | ||
|
|
||
| fn write_transformed_coord<C>( | ||
| writer: &mut impl Write, | ||
| coord: C, | ||
| affine: &DAffine, | ||
| dim: &geo_traits::Dimensions, | ||
| ) -> Result<()> |
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) It may be unfounded, but my intuition would be that match (dim) should be in write_transformed_coords() to avoid a match() for every coordinate. Perhaps the compiler does this anyway or the transform operation is sufficiently expensive that it doesn't matter 🙂
| pub(crate) struct DAffine2Iterator<'a> { | ||
| index: usize, | ||
| a: &'a PrimitiveArray<Float64Type>, | ||
| b: &'a PrimitiveArray<Float64Type>, | ||
| d: &'a PrimitiveArray<Float64Type>, | ||
| e: &'a PrimitiveArray<Float64Type>, | ||
| x_offset: &'a PrimitiveArray<Float64Type>, | ||
| y_offset: &'a PrimitiveArray<Float64Type>, | ||
| } |
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.
Do we need multiple versions of this (e.g., can the core logic be distilled to struct NumericArgs { args: Vec<&'a PrimitiveArray<Float64Type>> }? Or maybe the lifetimes make this difficult?).
My reading of these is that they don't handle nulls in any of the arguments. I am not sure if it's optimizing this but you could check ahead of time if the null_count() is >0 for any of the inputs and use the slower iterator if this is the case.
Co-authored-by: Dewey Dunnington <[email protected]>
yutannihilation
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.
Thanks for reviewing (and sorry for a bit massive PR)! I'll try using CrsTransform.
| let executor = WkbExecutor::new(arg_types, args); | ||
| let mut builder = BinaryBuilder::with_capacity( | ||
| executor.num_iterations(), | ||
| WKB_MIN_PROBABLE_BYTES * executor.num_iterations(), |
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.
Ah, true. Thanks for catching.
| match geom.as_type() { | ||
| geo_traits::GeometryType::Point(pt) => { | ||
| if pt.coord().is_some() { | ||
| write_wkb_point_header(writer, dims) | ||
| .map_err(|e| DataFusionError::Execution(e.to_string()))?; | ||
| write_transformed_coord(writer, pt.coord().unwrap(), mat, dim)?; | ||
| } else { | ||
| write_wkb_empty_point(writer, dims) | ||
| .map_err(|e| DataFusionError::Execution(e.to_string()))?; | ||
| } | ||
| } | ||
|
|
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.
Sounds good to me. I'll try.
(As translation can also be handled by affine transformation, I was wondering if ST_Transform can also be implemented using glam. But, I guess there will be not much difference)
|
I think I addressed most of the comments. Thanks for the advices! |
paleolimbot
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.
I tried to get through all of this before the end of the day but I may have missed some things. Thank you for these edits...I think it is very close!
Co-authored-by: Dewey Dunnington <[email protected]>
paleolimbot
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.
Thank you!
|
Thanks so much for reviewing! |
This pull request implements
ST_Affine(),ST_RotateandST_Scaleusing the glam crate. I chose glam becausebut I don't have strong opinion. If it is desirable to implement from scratch, I can do so.
ST_Affine()ST_Rotate()angle and origin(I'll address in another pull request)ST_Scale()scale and origin(I'll address in another pull request)