fix(c/sedona-s2geography): Make geography ring orientation less surprising for rings with crossing edges

[jiayuasu]

Description

Fixes #1085.

Rings with crossing edges are invalid, and geography behaviour for them is undefined — but the default ST_GeogFromWKB/ST_GeogFromWKT accepts them, and the orientation heuristic could invert such a polygon's interior to cover nearly the entire sphere: ST_Area returned a negative value and ST_Within/ST_Intersects matched points anywhere on Earth, silently corrupting every spatial join against the table. This change makes the orientation heuristic derive the winding direction in a way that stays correct for this kind of input, at no cost to valid input.

Root cause

Every geography UDF materializes WKB through GeoArrowGeography::Init, which calls GeoArrowLaxPolygonShape::NormalizeOrientation() to make shells counterclockwise and holes clockwise. That function derived each ring's winding direction from the sign of its curvature (turning angle). The affected ring is digitized with an out-and-back tail whose return path is offset by ~10 m; interpreted as geodesics, the return path crosses the outgoing path once. The crossing collapses the ring's turning number to zero, making the curvature sign meaningless, so the shell was reversed in place and its interior inverted. (This also explains why ST_Reverse/ST_Normalize had no effect: orientation is re-derived, and re-broken, on every evaluation.)

Change

paleolimbot/s2geography#125 updates NormalizeOrientation() to trust the curvature only when it is unambiguous and consult the ring's signed area otherwise: a valid ring has |curvature| == |2π − area(left side)|, so its curvature can only fall within (−π, π) when the ring encloses between a quarter and three quarters of the sphere. Outside that band the curvature sign is trusted, so the common case costs exactly what it costs today; inside it — unusually large valid rings, or invalid rings whose crossing edges collapse the turning number toward zero — the signed area decides, which reflects the ring's net winding direction even for self-crossing input. For every valid ring the decisions are identical to before.

This PR bumps the s2geography submodule to that change, adds a Python regression test with a minimal self-crossing sliver ring, and adds st_area benchmarks for Polygon(500) and PolygonWithHole(500) (the existing benchmark only covered Polygon(10)).

paleolimbot/s2geography#125 has merged, and the submodule now points at the merged upstream commit (3ab3dd9).

Benchmarks

Criterion, ST_Area over batches of random valid polygons (Apple Silicon), relative to the curvature-only baseline at the current submodule pin:

benchmark	curvature (baseline)	signed area only	curvature + signed-area fallback (this PR)
st_area Polygon(10)	140.4 ms	+36.1%	+3.4%
st_area Polygon(500)	6.95 s	+114% (CI +73% … +160%)	+4.4%
st_area PolygonWithHole(500)	14.29 s	+55.1%	−0.8% (p = 0.66, no change)

Verification

Against the original 258-point ring from #1085:

Expression	Before	After
ST_Area(geog)	−11 876 076 760.37	+11 876 076 760.37
ST_Perimeter(geog)	574 613.63	574 613.63 (unchanged)
ST_Within(POINT (-150 0), geog)	true	false
ST_Within(POINT (33.5 16.5), geog)	—	true

• s2geography C++ suite: 947/947 (new regression tests fail without the change, pass with it)
• cargo test -p sedona-s2geography: 68/68
• pytest tests/geography/: 1456 passed

Also filed #1154 for a geography ST_IsValid/ST_MakeValid as the principled follow-up for repairing this class of input.

[paleolimbot]

Can you check cargo bench -p s2geography (possibly adding a new benchmark for Scalar st_area with a large number of vertices / polygon with a hole)? I am worried using area vs curvature will impact that result (although it may even make it faster). Because the affected code gets run for all geography input it will affect all functions (so we should at least measure what happens here).

I would also tweak the phrasing of the title here...this is not fixing anything (invalid input causes undefined behaviour...also true for geometry with crossing edges), but does make geography functions less surprising for the current default behaviour of ST_ToGeography()

[jiayuasu]

Good call on both counts.

I added st_area benchmarks for Polygon(500) and PolygonWithHole(500) (the existing one only covered Polygon(10)) and measured three variants of the orientation check, all against the curvature baseline at the current submodule pin:

benchmark	curvature (baseline)	signed area only	curvature + signed-area fallback
st_area Polygon(10)	140.4 ms	+36.1%	+3.4%
st_area Polygon(500)	6.95 s	+114% (CI +73% … +160%)	+4.4%
st_area PolygonWithHole(500)	14.29 s	+55.1%	−0.8% (p = 0.66, no change)

So it did not make it faster — the signed area is ~3× the cost of the curvature per ring, and using it unconditionally is a real regression. I've updated paleolimbot/s2geography#125 with a hybrid instead: trust the curvature when |curvature| ≥ π (a valid ring can only fall inside that band when it encloses between ¼ and ¾ of the sphere), and consult the signed area only within the band. Valid rings get identical decisions and identical cost to today's code; the self-crossing ring from #1085 lands in the band and stays corrected. With that revision the benchmarks are at noise level.

Also retitled the PR per your phrasing note — agreed this is about making the default behaviour less surprising for input whose behaviour is undefined, not fixing defined behaviour.

[paleolimbot]

Thank you!

Just double checking that the submodule points to paleolimbot/s2geography and not a fork (that doesn't show up in the diff for some reason).

[jiayuasu]

Yes — .gitmodules still points at https://github.com/paleolimbot/s2geography.git