Teodor, et al, This is a review of the Polygons patch: http://archives.postgresql.org/message-id/4a5761a2.8070...@sigaev.ru
There hasn't been any discussion, at least that I've been able to find. Contents & Purpose ================== This small patch primarily fixes a couple polygon functions, poly_overlap (the && operator) and poly_contain (@>). Previously the functions only performed simple bounding box calculations or checks based on sets of points. That works only for the simplest of cases; this patch accounts for more complex shapes. Included in the patch are new regression test cases, but no changes to documentation. The patch only corrects the behavior of existing functions, though, so perhaps no changes to the documentation are expected. Initial Run =========== The patch applies cleanly to HEAD. The regression tests all pass successfully against the new patch, but fail against pre-patched HEAD, so the test cases are sane and do cover the new behavior. As far as I can see the math behind the new calculations seems sound. Performance =========== Despite the functions in question containing an order of magnitude more code the operators performed faster than the previous versions in my test run. Though I have a feeling that may have more to do with this laptop's processor speed and/or the rather trivial test cases being thrown at it. In any case having the operators work correctly should far outweigh the negligible performance impact. Nitpicking & Conclusion ======================= The patch splits out and adds a couple helper functions next to the existing ones in geo_ops.c, but would those be better defined down in the Private routines section? There's a #define in the middle of the touched_lseg_inside_poly() function. The macro is only used in that function and a couple of times at that, but it still feels somewhat out of place. Perhaps that'd be better placed with other #define's near the top? I could certainly be wrong in both cases. :) There's also two "int i"s declared in poly_contain(). Otherwise it seems to do exactly what it promises. I could see the correct behavior of these operators being important for GIS applications. +1 for committer review. - Josh Williams -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers