st_linesubstring func#777
st_linesubstring func#777sapienza88 wants to merge 10 commits intoapache:mainapache/sedona-db:mainfrom
Conversation
paleolimbot
left a comment
There was a problem hiding this comment.
Thanks for opening!
- This needs
cargo fmtto (and all the problems found withcargo clippyto be fixed). - This will need tests in Rust to verify the basic behaviour of the function. You can use one of the other functions in this directory as a template for the tests you'll need (basically ensure that all the branches of the implementation are covered)
- The new function needs to be documented in
docs/reference/sql - The new function needs Python integration tests, which is where we check the behaviour against PostGIS.
I think there are links for some of those in the initial ticket you filed but let me know if you get lost!
|
@paleolimbot I added some tests and the doc for this function, not sure I understand: "This needs cargo fmt to (and all the problems found with cargo clippy to be fixed)." |
paleolimbot
left a comment
There was a problem hiding this comment.
cargo fmt is something you can run from the terminal that normalizes the formatting of the rust code. It is also run automatically by pre-commit run --all-files. There's a CI check to make sure the pre-commit command runs cleanly, so if that check fails you'll have to run that locally to get it to pass.
|
|
||
| if expected is not None: | ||
| expected = expected.replace(", ", ",") | ||
| expected = expected.replace(" (", "(") | ||
|
|
||
| if isinstance(eng, PostGIS): | ||
|
|
||
| expected = expected.replace("Z(", "Z (") | ||
| expected = expected.replace("M(", "M (") | ||
| expected = expected.replace("ZM(", "ZM (") | ||
|
|
||
| eng.assert_query_result( | ||
| f"SELECT ST_AsText(ST_LineSubstring({geom_or_null(geom)}, {start}, {end}))", | ||
| expected, | ||
| ) No newline at end of file |
There was a problem hiding this comment.
You should be able to just feed the WKT directly into the expected slot, which should help with the difference between the WKT output:
| if expected is not None: | |
| expected = expected.replace(", ", ",") | |
| expected = expected.replace(" (", "(") | |
| if isinstance(eng, PostGIS): | |
| expected = expected.replace("Z(", "Z (") | |
| expected = expected.replace("M(", "M (") | |
| expected = expected.replace("ZM(", "ZM (") | |
| eng.assert_query_result( | |
| f"SELECT ST_AsText(ST_LineSubstring({geom_or_null(geom)}, {start}, {end}))", | |
| expected, | |
| ) | |
| eng.assert_query_result( | |
| f"SELECT ST_LineSubstring({geom_or_null(geom)}, {start}, {end})", | |
| expected, | |
| ) | |
paleolimbot
left a comment
There was a problem hiding this comment.
Sorry for being slow to review this!
I left a few suggestions about how to support Z and M right out of the gate (since I saw that test was failing and it's a nice feature to be able to support).
The clippy error you should be able to fix with cargo clippy --fix (it's an unused import).
| @pytest.mark.parametrize( | ||
| ("geom", "start", "end", "expected"), | ||
| [ | ||
| (None, 0.0, 1.0, None), |
There was a problem hiding this comment.
| (None, 0.0, 1.0, None), | |
| (None, 0.0, 1.0, None), | |
| ("LINESTRING EMPTY", None, 1.0, None), | |
| ("LINESTRING EMPTY, 0.0, None, None), |
| fn interpolate<C: CoordTrait<T = f64>>(p1: C, p2: C, fraction: f64) -> (f64, f64) { | ||
| let x = p1.x() + (p2.x() - p1.x()) * fraction; | ||
| let y = p1.y() + (p2.y() - p1.y()) * fraction; | ||
| (x, y) | ||
| } |
There was a problem hiding this comment.
You have a test for Z that's failing because this doesn't handle anything except XY coordinates. I think it should be relatively easy to modify this to work with any dimension...maybe:
fn interpolate<C: CoordTrait<T = f64>>(p1: C, p2: C, fraction: f64, dim: Dimensions, buf: &mut impl Write) -> Result<(), SedonaGeometryError> {
for i in 0..dim.size() {
let v = p1.nth_unchecked(i) + (p2.nth_unchecked(i) - p1.nth_unchecked(i)) * fraction;
buf.write_all(v.to_le_bytes())?;
}
Ok(())
}| } | ||
|
|
||
| if d1 > start_dist && d1 < end_dist { | ||
| new_coords.push((p1.x(), p1.y())); |
There was a problem hiding this comment.
| new_coords.push((p1.x(), p1.y())); | |
| write_wkb_coord_trait(p1, &mut coords)?; |
| } else { | ||
| 0.0 | ||
| }; | ||
| new_coords.push(interpolate(p1, p2, fraction)); |
There was a problem hiding this comment.
| new_coords.push(interpolate(p1, p2, fraction)); | |
| new_coords.push(interpolate(p1, p2, fraction, line.dim(), &mut coords)?); |
| if !new_coords.is_empty() { | ||
| // write byte order (1 = little endian) | ||
| builder.write_all(&[1u8])?; | ||
|
|
||
| let type_id: u32 = 2; | ||
| builder.write_all(&type_id.to_le_bytes())?; | ||
|
|
||
| let num_points = new_coords.len() as u32; | ||
| builder.write_all(&num_points.to_le_bytes())?; | ||
|
|
||
| for (x, y) in new_coords { | ||
| builder.write_all(&x.to_le_bytes())?; | ||
| builder.write_all(&y.to_le_bytes())?; | ||
| } |
There was a problem hiding this comment.
You can use write_wkb_linestring_header(&mut builder, line.dim(), num_output_coords)? for this (there are other utility functions in sedona_geometry::wkb_factory that might be useful as well. I've added suggestions above about how to accumulate the coordinates as bytes directly in your implementation (which also simplifies supporting Z and M here).
There was a problem hiding this comment.
I’m a little rusty when it comes to Rust, but I’ll try to use some AI tools to help make this mergeable.
No description provided.