Opened 14 months ago

Closed 3 months ago

#5361 closed defect (fixed)

Compound Curve inconsistent behavior with ST_GeometryN, ST_NumGeometries, ST_Dump, and ST_DumpSegments

Reported by: robe Owned by: pramsey
Priority: low Milestone: PostGIS 3.5.0
Component: postgis Version: master
Keywords: Cc:

Description

Someone asked a question on IRC, which caused me some concern.

How do you get the child types of a compound curve?

I was bothered because of the dichotomy of functions that should agree with each other that don't

SELECT ST_Numgeometries(geom), ST_GeometryType(ST_GeometryN(geom,1)), ST_GeometryN(geom,2)
FROM ST_GeomFromText(
'COMPOUNDCURVE(
(2 2, 2.5 2.5),
CIRCULARSTRING(2.5 2.5, 4.5 2.5, 3.5 3.5),
(3.5 3.5, 2.5 4.5, 3 5)
)') AS f(geom);

Output is:

st_numgeometries | st_geometrytype  | st_geometryn
------------------+------------------+--------------
                3 | ST_CompoundCurve |
(1 row)

One can argue both sides, that a compoundcurve is no different from a LINESTRING in that it defines a contiguous line of sorts.

But yet both ST_Numgeometries and ST_Dump do not see it that way

SELECT gd.path,  ST_GeometryType(gd.geom)
FROM ST_GeomFromText(
'COMPOUNDCURVE(
(2 2, 2.5 2.5),
CIRCULARSTRING(2.5 2.5, 4.5 2.5, 3.5 3.5),
(3.5 3.5, 2.5 4.5, 3 5)
)') AS f(geom), ST_Dump(f.geom) AS gd;
path |  st_geometrytype
------+-------------------
 {1}  | ST_LineString
 {2}  | ST_CircularString
 {3}  | ST_LineString
(3 rows)

But then if ST_Dump were not flawed in this one, one could not arrive at the subtypes of a compound curve. What of ST_NumGeometries? Do the specs say anything about how compound curve sub-elements should be counted.

Change History (11)

comment:1 by pramsey, 12 months ago

Perhaps it is ST_GeometryN that needs the fix…

Yes, a compoundcurve is "one thing", but it could also be considered "three things with some rules" (namely the endpoint-to-endpoint coordinate matching). Linear ring is just a linestring with an extra rule.

So I guess it should be possible to deconstruct a CompoundCurve with ST_GeometryN? Certainly since we can dump it already.

comment:2 by robe, 12 months ago

I suspect trying to get the others to do less is more likely to break people's workflow than having ST_GeometryN do more or maybe we should do nothing :)

Out of curiosity I tried this on a SQL Server 2017 server.

WITH a AS (SELECT Geometry::STGeomFromText(
'COMPOUNDCURVE(
(2 2, 2.5 2.5),
CIRCULARSTRING(2.5 2.5, 4.5 2.5, 3.5 3.5),
(3.5 3.5, 2.5 4.5, 3 5)
)',0) AS geom )

SELECT a.geom.STGeometryN(2), a.geom.STNumGeometries(), a.geom.STNumCurves(), a.geom.STCurveN(2).STAsText()
FROM a;

and it returned

NULL, 1, 4 , CIRCULARSTRING (2.5 2.5, 4.5 2.5, 3.5 3.5)

which in my mind looks like the correct way to handle this. But is it worth it?

What I'm mad about is we don't have an ST_NumCurves and ST_CurveN function. How did we live so long without these?

I trashed my Oracle server so not sure what answer that would give. Maybe we just leave it alone since no one has complained except the IRC guy. If they do complain we could introduced a ST_CurveN and ST_NumCurves functions that counts the components in the curve.

comment:3 by robe, 11 months ago

Milestone: PostGIS 3.4.0PostGIS 3.5.0

I'm tableing this for now. Not sure it's worth doing anything about.

comment:4 by robe, 10 months ago

Summary: Compound Curve inconsistent behavior with ST_GeometryN, ST_NumGeometries, and ST_DumpCompound Curve inconsistent behavior with ST_GeometryN, ST_NumGeometries, and ST_Dump, add support for SQL MM ST_NumCurves and ST_CurveN

comment:5 by pramsey, 9 months ago

So, the offending piece of code is this one

	/* call is valid on multi* geoms only */
	if (type==POINTTYPE     || type==LINETYPE    || type==CIRCSTRINGTYPE ||
	    type==COMPOUNDTYPE  || type==POLYGONTYPE ||
	    type==CURVEPOLYTYPE || type==TRIANGLETYPE)
	{
		if ( idx == 0 ) PG_RETURN_POINTER(geom);
		PG_RETURN_NULL();
	}

Both COMPOUNDTYPE and CURVEPOLYTYPE are, structurally, multi-geometries, so it would be easy to cast them to COLLECTION hand them off… then a CURVEPOLYGON would be handled like a nested GEOMETRYCOLLECTION… the rings would each be a COMPOUND and each of those would in turn be a set of CURVESTRING and/or LINESTRING.

This seems… not entirely bad? And it does match the ST_Dump behaviour?

Last edited 9 months ago by pramsey (previous) (diff)

comment:6 by robe, 9 months ago

Can I have a ST_NumCurves and ST_CurveN too.

I think at the very least ST_NumGeometries and ST_GeometryN should at least agree with each other which is what your proposed change would fix at least.

If ST_NumGeometries says there are 3 geometries, then I should be able to do ST_GeometryN(geom, 3) and get something back. That does seem like it would cause less friction than changing the behavior to 1 geometry.

Then again, how many people would be using curves to care how that changes unless everyone gets into the habit now of SVGing their curves now that we support that.

That said, I feel SQL Server is more the expected behavior and their having a ST_NumCurves, ST_CurveN to handle the subelements.

I'm afraid to ask what it does with PolyhedralSurfaces and TINS. I'm pretty sure ST_Dump expands those to polygons and triangles. In theory one can argue are those really any different from multipolygons? To me they are.

What I was thinking of was an extract argument expand_complex=true

SO SELECT ST_NumGeometries(geom) would do as it does now

SELECT ST_GeometryN(geom) — would actually agree with ST_NumGeometries

But if I did SELECT ST_NumGeometries(geom, expand_complex ⇒ false);

SELECT ST_GeometryN(geom, expand_complex ⇒ false);

My curved stuff and TIN stuff and PolyhedralSurface stuff would be treated as one unit.

So that way it's backward compatible except for the case where it was broken anyway.

We could add the same arg to ST_Dump(geom, expand_complex ⇒ false)

cause yes I have on occasion wanted to expand my TINS and polyhedralsurfaces and curved stuff into the subelements, but many times I want to treat them as a single unit.

comment:7 by robe, 3 months ago

Summary: Compound Curve inconsistent behavior with ST_GeometryN, ST_NumGeometries, and ST_Dump, add support for SQL MM ST_NumCurves and ST_CurveNCompound Curve inconsistent behavior with ST_GeometryN, ST_NumGeometries, and ST_Dump

comment:8 by robe, 3 months ago

Another instance ST_DumpSegments should either say it doesn't work with compound curves, or do something useful

SELECT dp.path, ST_AsText(dp.geom)
FROM ST_DumpSegments('COMPOUNDCURVE(
(2 2, 2.5 2.5),
CIRCULARSTRING(2.5 2.5, 4.5 2.5, 3.5 3.5),
(3.5 3.5, 2.5 4.5, 3 5)
)'::geometry) AS dp;

path | st_astext
------+-----------
(0 rows)

Doesn't give an error but returns no rows.

comment:9 by robe, 3 months ago

Summary: Compound Curve inconsistent behavior with ST_GeometryN, ST_NumGeometries, and ST_DumpCompound Curve inconsistent behavior with ST_GeometryN, ST_NumGeometries, ST_Dump, and ST_DumpSegments

comment:10 by pramsey, 3 months ago

Some notes from a consistency check. The main question philosophically is whether CompoundCurve and CurvePolygon should be treated as unitary objects (like LineString and Polygon) or as collections. Either interpretation can result in consistent behaviour, I think.

https://gist.github.com/pramsey/cbf6db1edb8762854925f6b6ad6d6b35

comment:11 by Paul Ramsey <pramsey@…>, 3 months ago

Resolution: fixed
Status: newclosed

In ec6ddc0/git:

Add ST_CurveN and ST_NumCurves. Make ST_GeometryN and ST_NumGeometries behaviour
consistent for compound curves. Fix ring accessors for ST_CurvePolygon.
Enhance ST_DumpSegments to correctly dump circular arcs, and support all
geometry types. Closes #5361

Note: See TracTickets for help on using tickets.