On Wed, Sep 20, 2017 at 10:00 PM, Darafei Praliaskouski <me@komzpa.net> wrote:
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation: tested, passed
Hi,
I like the SP-GiST part of the patch. Looking forward to it, so PostGIS can benefit from SP-GiST infrastructure.
I have some questions about the circles example though.
* What is the reason for isnan check and swap of box ordinates for circle? It wasn't in the code previously.
* There are tests for infinities in circles, but checks are for NaNs.
This code was migrated from original patch by Nikita. I can assume he means that nan should be treated as greatest possible floating point value (like float4_cmp_internal() does). However, our current implementation of geometrical datatypes don't correctly handles all the combinations of infs as nans. Most of code was written without taking infs and nans into account. Also, I'm not sure if this code fixes all possible issues with infs and nans in SP-GiST opclass for circles. This is why I'm going to remove nans handling from this place.
* It seems to me that circle can be implemented without recheck, by making direct exact calculations.
Right. Holding circles in the leafs instead of bounding boxes would both allow exact calculations and take less space.
How about removing circle from the scope of this patch, so it is smaller and cleaner?
Good point. Polygons are enough for compress function example. Opclass for circles could be submitted as separate patch.
