feat(c/sedona-geos): Implement ST_IsRing

[joonaspessi]

Hello!

Thought that implementing something relatively simple from your first issue proposal would be a good way to understand how sedona-db works. Thanks for creating this project.

This PR implements ST_IsRing with GEOS.

I did some research on how other spatial libraries handle this function when a non-LineString geometry is passed:

System	Non-LineString Behavior
DuckDB spatial extensions	Returns false
Apache Sedona	Returns null
PostGIS	Throws error
This PR	Returns false

I chose to follow the original Apache Sedona rather than PostGIS because it matches the existing Sedona ecosystem.

[petern48]

Welcome! I left a few comments, but it looks good so far. Just need to agree on which behavior to follow.

[petern48]

Suggested change

	// Check if geometry is a LineString
	let geom_type = geos_geom.geometry_type();
	// ST_IsRing only applies to LineStrings - return false for other types
	// This matches DuckDB spatial extension and Apache Sedona behavior
	if geom_type != GeometryTypes::LineString {
	return Ok(false);
	}
	// Check if the LineString is closed
	let is_closed = geos_geom.is_closed().map_err(|e| {
	DataFusionError::Execution(format!("Failed to check if geometry is closed: {e}"))
	})?;
	// Check if the LineString is simple (no self-intersections)
	let is_simple = geos_geom.is_simple().map_err(|e| {
	DataFusionError::Execution(format!("Failed to check if geometry is simple: {e}"))
	})?;
	Ok(is_closed && is_simple)
	Ok(geos_geom.is_ring().map_err(|e| {
	DataFusionError::Execution(format!("Failed to check if geometry is a ring: {e}"))
	})?)

We can actually simplify this code to simply call geos's .is_ring() method here. Seems to pass everything when I tried this locally.

[petern48]

Suggested change

	("LINESTRING EMPTY", False),
	("LINESTRING EMPTY", False),
	# Collections of linestrings that would have been considered rings
	("MULTILINESTRING((0 0, 0 1, 1 1, 1 0, 0 0))", False),
	("GEOMETRYCOLLECTION(LINESTRING(0 0, 0 1, 1 1, 1 0, 0 0))", False),

Let's add these two cases. Returning False is the natural behavior for the geos's is_ring() method, which agrees with DuckDB

[joonaspessi]

Makes sense. Added these under test_st_isring_non_linestring as PostGis is throwing error on these.

[petern48]

I personally agree with the proposed behavior. PostGIS's implementation is odd imo. For non-linestrings, it returns ERROR: ST_IsRing() should only be called on a linear feature if it's non-empty, but returns False if it's empty. The thing I don't like about this is that if you have any non-empty non-linestring geometry in your column, your query will just fail.

CREATE TABLE geom (geom GEOMETRY);
INSERT INTO geom VALUES (ST_GeomFromText('LINESTRING EMPTY')), (ST_Point(3, 4));
SELECT st_isring(geom) FROM geom;
-- ERROR:  ST_IsRing() should only be called on a linear feature

DuckDB returns False instead of the error, which agrees with the natural behavior of the is_ring() function. Sedona actually returns NULL for any non-linestring geometry (empty or not).

WDYT @paleolimbot?

[paleolimbot]

Thank you!

DuckDB returns False instead of the error, which agrees with the natural behavior of the is_ring() function. Sedona actually returns NULL for any non-linestring geometry (empty or not).

I think we should follow PostGIS here and file a bug in Sedona-spark to discuss if that decision was intentional. If a user is trying to check the ring status of something that isn't even the correct geometry type there is likely an error in the query or an error in the data.

In the next DataFusion release we'll have the ability to pipe options into scalar functions and so we can do things like enable a Sedona-spark compatibility mode. Here I think PostGIS compatibility is the right choice because it's the easiest to test and review (and in general has more intentional corner case behaviour.

[jiayuasu]

Fine by me. PostGIS compatibility is more important

[joonaspessi]

Thanks for the comments. I think the PostGis compatibility is a good choice to follow. Updated the PR to follow that approach. While doing it, also noticed that for empty geometries, PostGis will return false.

[paleolimbot]

Just one nit on the exception check (which should also fix the pre-commit)...thank you!

[paleolimbot]

I think this will work:

Suggested change

	with pytest.raises(Exception) as exc_info:
	eng.assert_query_result(f"SELECT ST_IsRing(ST_GeomFromText('{geom}'))", None)
	# Verify error message contains the expected text
	error_msg = str(exc_info.value).lower()
	assert (
	"linear" in error_msg or "linestring" in error_msg
	), f"Expected error about linear feature, got: {exc_info.value}"
	with pytest.raises(Exception, match="linear|linestring") as exc_info:
	eng.assert_query_result(f"SELECT ST_IsRing(ST_GeomFromText('{geom}'))", None)

[joonaspessi]

Good idea! Updated to cleanly utilize pytest match pattern. Pre-commit passes locally

[paleolimbot]

Thank you!