Re: compress method for spgist - 2

On Wed, Sep 20, 2017 at 10:00 PM, Darafei Praliaskouski <me(at)komzpa(dot)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.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company