Implement ST_InteriorRingN

[Abeeujah]

This PR implements ST_InteriorRingN using geo-traits as described in #224 And adds support for Int64 in BenchmarkArgs

[paleolimbot]

Thank you!

I believe the CI failure should be fixed by #382 and isn't related to this PR.

[paleolimbot]

I think the writing of the output should be more like the output writing you added for ST_Reverse:

sedona-db/rust/sedona-functions/src/st_reverse.rs

Lines 189 to 190 in 16d232b

	coords.rev().try_for_each(|coord| {
	write_wkb_coord_trait(writer, &coord).map_err(|e| DataFusionError::Execution(e.to_string()))

...in particular, I am not sure that to_line_string() and x_y() are what we want here because we need to support Z, M, and ZM coordinate types.

Also, wkb might be a better name than buf here since I don't think buf is actually a buffer here

[Abeeujah]

polygon.interior() returns a LinearRing which is Wkb coded differently from Linestring, though the coords are the same, the test tends to fail since it returns a linear ring instead of a linestring, hence the reason for the to_linestring() there.

Would update the logic there. 👍 and use write_wkb_coord_trait instead 👍

[paleolimbot]

Thank you for all of these!

I think the missing cases here are Z, M, and ZM. It may be helpful to break these up into a few related input_wkt/inters/expected triplets to keep the whole test case roughly on your screen as you're scrolling.

[Abeeujah]

Would add them, and separate into groups 👍

[paleolimbot]

I'm not sure this is a very representative benchmark because it sould return NULL for every input (Polygon(10) doesn't have a hole). It may be less confusing to omit it here (rather than include a version that isn't measuring what it appears to be measuring).

[Abeeujah]

Is there a way to guage and know which would have a hole? say Polygon(500)?

[paleolimbot]

We'd need a new BenchmarkArgSpec (maybe PolygonWithHole) here:

sedona-db/rust/sedona-testing/src/benchmark_util.rs

Lines 275 to 276 in 16d232b

	/// Randomly generated polygon input with a specified number of vertices
	Polygon(usize),

...and it would need to be added to build_geometry():

sedona-db/rust/sedona-testing/src/benchmark_util.rs

Lines 421 to 429 in 16d232b

	fn build_geometry(
	&self,
	i: usize,
	geom_type: GeometryTypeId,
	num_batches: usize,
	vertex_count: usize,
	num_parts_count: usize,
	rows_per_batch: usize,
	) -> Result<Vec<ArrayRef>> {

(the polygon hole rate is something you can specify to the random partitioned data builder)

[paleolimbot]

This is looking great! Feel free to punt on the benchmark data suggestion (but if you do, perhaps remove the benchmark so that we don't leave a misleading one in)

[paleolimbot]

Can you add the Z, M, and ZM cases you generated below to this list as well?

[paleolimbot]

Thanks!

There's one clippy error we need to fix to keep CI happy (I put a suggestion inline).

[paleolimbot]

This caused clippy to complain about too many arguments. While clippy is right here (the call to build_geometry is getting a bit unwieldy/unreadable), it's an internal function and it's well tested. You can add #[allow(clippy::too_many_arguments)] right above fn build_geometry( to fix the error.

[petern48]

Testing a negative index case would be nice