jiayuasu commented on PR #1143: URL: https://github.com/apache/sedona-db/pull/1143#issuecomment-5259303520
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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
