Re: Review: Patch for contains/overlap of polygons

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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +