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.