Re: compress method for spgist - 2

On Wed, Sep 20, 2017 at 11:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Darafei Praliaskouski <me@komzpa.net> writes:
> > 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.
>
> I hadn't paid any attention to this patch previously, but this comment
> excited my curiosity, so I went and looked:
>
> +       bbox->high.x = circle->center.x + circle->radius;
> +       bbox->low.x = circle->center.x - circle->radius;
> +       bbox->high.y = circle->center.y + circle->radius;
> +       bbox->low.y = circle->center.y - circle->radius;
> +
> +       if (isnan(bbox->low.x))
> +       {
> +               double tmp = bbox->low.x;
> +               bbox->low.x = bbox->high.x;
> +               bbox->high.x = tmp;
> +       }
>
> Maybe I'm missing something, but it appears to me that it's impossible for
> bbox->low.x to be NaN unless circle->center.x and/or circle->radius is a
> NaN, in which case bbox->high.x would also have been computed as a NaN,
> making the swap entirely useless.  Likewise for the Y case.  There may be
> something useful to do about NaNs here, but this doesn't seem like it.
>

Yeah, +1.

> How about removing circle from the scope of this patch, so it is smaller
> and cleaner?
>
> Neither of those patches would be particularly large, and since they'd need
> to touch adjacent code in some places, the diffs wouldn't be independent.
> I think splitting them is just make-work.
>

I've extracted polygon opclass into separate patch (attached).  I'll rework
and resubmit circle patch later.
I'm not particularly sure that polygon.sql is a good place for testing
sp-gist opclass for polygons...  But we've already done so for box.sql.

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