st_linesubstring func

[sapienza88]

No description provided.

[paleolimbot]

Thanks for opening!

• This needs cargo fmt to (and all the problems found with cargo clippy to 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!

[sapienza88]

@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]

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.

[paleolimbot]

You should be able to just feed the WKT directly into the expected slot, which should help with the difference between the WKT output:

Suggested change

	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]

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).

[paleolimbot]

Suggested change

	(None, 0.0, 1.0, None),
	(None, 0.0, 1.0, None),
	("LINESTRING EMPTY", None, 1.0, None),
	("LINESTRING EMPTY, 0.0, None, None),

[paleolimbot]

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(())
        }

[paleolimbot]

Suggested change

	new_coords.push((p1.x(), p1.y()));
	write_wkb_coord_trait(p1, &mut coords)?;

[paleolimbot]

Suggested change

	new_coords.push(interpolate(p1, p2, fraction));
	new_coords.push(interpolate(p1, p2, fraction, line.dim(), &mut coords)?);

[paleolimbot]

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).

[sapienza88]

I’m a little rusty when it comes to Rust, but I’ll try to use some AI tools to help make this mergeable.