Thread: 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@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. +
On Sun, Aug 9, 2009 at 2:20 PM, Bruce Momjian<bruce@momjian.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@momjian.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@momjian.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@ij.net> wrote: > How's that for over-engineering? ;) Top notch. ...Robert