Re: KNN searches support for SP-GiST [GSOC'14]

On 08/20/2014 03:35 AM, Vladislav Sterzhanov wrote:
> Hi there, pg-Hackers!
> Here I go with the patch which brings up the possibility to perform
> nearest-neighbour searches on SP-GiSTs (as of now includes implementation
> for quad and kd trees). Pre-reviewed by my GSoC mentor Alexander Korotkov.
> Sample benchmarking script included in the attachment (dumps the current
> geonames archive and runs several searches on the (latitude, longitude)
> points), which demonstrates the dramatic improvements against plain
> searches and sorting.

Cool!

I think this needs a high-level description in access/spgist/README on
how this works. You can probably copy-paste the similar description from
gist's README, because it's the same design. If I understand correctly,
the support functions return distances between the nodes and the query,
and the SP-GiST code uses those distances to return the rows in order.
An important detail there is that the distance returned for an inner
node is a lower bound of the distance of any node in that branch.

A binary heap would be a better data structure than a red-black tree for
the queue of nodes to visit/return. I know that GiST also uses a
red-black for the same thing, but that's no reason not to do better
here. There is a binary heap implementation in the backend too (see
src/backend/lib/binaryheap.c), so it's not any more difficult to use.

Does the code handle ties correctly? The distance function is just an
approximation, so it's possible that there are two items with same
distance, but are not equal according to the ordering operator. As an
extreme example, if you hack the distance function to always return 0.0,
do you still get correct results (albeit slowly)?

The new suppLen field in spgConfigOut is not mentioned in the docs. Why
not support pass-by-value supplementary values?

A couple of quick comments on the code:

> @@ -137,8 +138,8 @@ spg_quad_picksplit(PG_FUNCTION_ARGS)
> {
> spgPickSplitIn *in = (spgPickSplitIn *) PG_GETARG_POINTER(0);
> spgPickSplitOut *out = (spgPickSplitOut *) PG_GETARG_POINTER(1);
> - int i;
> - Point *centroid;
> + int i;
> + Point *centroid;
>
> #ifdef USE_MEDIAN
> /* Use the median values of x and y as the centroid point */

Please remove all unnecessary whitespace changes like this.

> @@ -213,14 +215,19 @@ spg_quad_inner_consistent(PG_FUNCTION_ARGS)
> }
>
> Assert(in->nNodes == 4);
> +
> + if (in->norderbys > 0)
> + {
> + out->distances = palloc(in->nNodes * sizeof (double *));
> + }

I think that should be "sizeof(double)".

> + *distances = (double *) malloc(norderbys * sizeof (double *));

No mallocs allowed in the backend, should be palloc.

- Heikki