Re: Review: Patch for contains/overlap of polygons - Mailing list pgsql-hackers

This is a nice section layout for a patch review report --- should we
provide an email template like this one for reviewers to use?

---------------------------------------------------------------------------

Josh Williams wrote:
> 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
> 
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +