|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
> 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.
> 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:
+ If your life is a hard drive, Christ can be your backup. +
|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|