Review: Patch for contains/overlap of polygons

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

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. +

On Sun, Aug 9, 2009 at 2:20 PM, Bruce Momjian<bruce(at)momjian(dot)us> wrote:
> This is a nice section layout for a patch review report --- should we
> provide an email template like this one for reviewers to use?

We could, but it might be over-engineering. Those particular section
headers might not be applicable to someone else's review.

...Robert

On Sun, Aug 09, 2009 at 02:29:44PM -0400, Robert Haas wrote:
> On Sun, Aug 9, 2009 at 2:20 PM, Bruce Momjian<bruce(at)momjian(dot)us> wrote:
> > This is a nice section layout for a patch review report --- should we
> > provide an email template like this one for reviewers to use?
>
> We could, but it might be over-engineering. Those particular section
> headers might not be applicable to someone else's review.

I've just added a link to this email to the "Reviewing a Patch" wiki page
(http://wiki.postgresql.org/wiki/Reviewing_a_Patch). Do with it as you see fit
:)

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

On Sun, 2009-08-09 at 13:27 -0600, Joshua Tolley wrote:
> On Sun, Aug 09, 2009 at 02:29:44PM -0400, Robert Haas wrote:
> > On Sun, Aug 9, 2009 at 2:20 PM, Bruce Momjian<bruce(at)momjian(dot)us> wrote:
> > > This is a nice section layout for a patch review report --- should we
> > > provide an email template like this one for reviewers to use?
> >
> > We could, but it might be over-engineering. Those particular section
> > headers might not be applicable to someone else's review.
>
> I've just added a link to this email to the "Reviewing a Patch" wiki page
> (http://wiki.postgresql.org/wiki/Reviewing_a_Patch). Do with it as you see fit
> :)

Sweet. :)

Actually that was mainly for keeping organized and sane when conducting
my first review, and it seemed to translate well into the email when it
came time to write it up.

The appropriate sections* most certainly would change patch-to-patch --
reviewer-to-reviewer, even -- so a set template wouldn't be appropriate.
But as a style recommendation it could make sense. I'd made a mental
note to try and refine the formatting next time around, but I haven't
been back to request another yet.

On that note, and now that I'm back online and clean of Pennsic dust,
anything else in this CommitFest in need of a last minute Windows
run-through?

- Josh Williams

* I could envision having the ability to write reviews directly into the
commitfest web app, where one could define and tag sections. Then
anyone curious about a patch's performance implications, for example,
could pull down and read just the performance results of potentially
multiple reviewers. How's that for over-engineering? ;)

On Sun, Aug 9, 2009 at 10:01 PM, Josh Williams<joshwilliams(at)ij(dot)net> wrote:
>  How's that for over-engineering? ;)

Top notch.

...Robert