Teodor, et al,
This is a review of the Polygons patch:
http://archives.postgresql.org/message-id/4A5761A2.8070602@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
