Re: Review: Patch for contains/overlap of polygons

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Josh Williams <joshwilliams(at)ij(dot)net>
Cc: Teodor Sigaev <teodor(at)sigaev(dot)ru>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Review: Patch for contains/overlap of polygons
Date: 2009-08-09 18:20:03
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: 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:
> 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:

Bruce Momjian <bruce(at)momjian(dot)us>

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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2009-08-09 18:27:44 Re: mixed, named notation support
Previous Message Tom Lane 2009-08-09 18:17:18 Re: mixed, named notation support