Re: [HACKERS] [PATCH] kNN for SP-GiST

On Thu, Sep 27, 2018 at 8:28 PM Mark Dilger <hornschnorter(at)gmail(dot)com> wrote:

> > On Sep 18, 2018, at 3:58 PM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> >
> > On Mon, Sep 17, 2018 at 12:42 PM Andrey Borodin <x4mmm(at)yandex-team(dot)ru>
> wrote:
> >>> 17 сент. 2018 г., в 2:03, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> написал(а):
> >>>
> >>> Also, it appears to me that it's OK to be a single patch
> >>
> >> +1, ISTM that these 6 patches represent atomic unit of work.
> >
> > Thank you, pushed.
>
> One note on this commit,
>
> in my fork, I have converted BoxPGetDatum from a macro to a static inline
> function,
> and it shows a thinko in your commit, namely that in->leafDatum is of type
> (BOX *),
> but is actually (as the name implies) already a Datum:
>
> spgquadtreeproc.c:469:27: error: incompatible integer to pointer
> conversion passing 'Datum' (aka 'unsigned long') to parameter of type 'BOX
> *' [-Werror,-Wint-conversion]
>
> BoxPGetDatum(in->leafDatum), true,
>
> I'm not claiming this creates any active bug, just that it would make more
> sense to
> handle the typing cleanly. Perhaps the following:
>
> diff --git a/src/backend/access/spgist/spgquadtreeproc.c
> b/src/backend/access/spgist/spgquadtreeproc.c
> index dee438a307..90cc776899 100644
> --- a/src/backend/access/spgist/spgquadtreeproc.c
> +++ b/src/backend/access/spgist/spgquadtreeproc.c
> @@ -465,8 +465,7 @@ spg_quad_leaf_consistent(PG_FUNCTION_ARGS)
>
> if (res && in->norderbys > 0)
> /* ok, it passes -> let's compute the distances */
> - out->distances = spg_key_orderbys_distances(
> -
> BoxPGetDatum(in->leafDatum), true,
> + out->distances = spg_key_orderbys_distances(in->leafDatum,
> true,
>
> in->orderbys, in->norderbys);
>

True. Pushed, thanks!

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