Re: Add missing operator <->(box, point)
On Mon, Jul 8, 2019 at 11:39 PM Nikita Glukhov wrote: > On 08.07.2019 18:22, Alexander Korotkov wrote: > For me it doesn't look worth having two distinct functions > gist_box_distance_helper() and gist_bbox_distance(). What about > having just one and leave responsibility for recheck flag to the > caller? > > gist_bbox_distance() was removed. OK. > But maybe it would be better to replace two identical functions > gist_circle_distance() and gist_poly_distance() with the single > gist_bbox_distance()? Sounds reasonable to me. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Add missing operator <->(box, point)
Attached 3rd version of the patches.
On 02.07.2019 21:55, Alexander Korotkov wrote:
On Tue, Jul 2, 2019 at 9:19 PM Nikita Glukhov wrote:
We could use commuted "const <-> var" operators for kNN searches, but the
current implementation requires the existence of "var <-> const" operators, and
order-by-op clauses are rebuilt using them (see match_clause_to_ordering_op()
at /src/backend/optimizer/path/indxpath.c).
But probably it's still worth to just add commutator for every <->
operator and close this question. Otherwise, it may arise again once
we want to add some more kNN support to opclasses or something. On
the other hand, are we already going to limit oid consumption?
All missing distance operators were added to the first patch.
On 08.07.2019 18:22, Alexander Korotkov wrote:
On Mon, Mar 11, 2019 at 2:49 AM Nikita Glukhov wrote:
2. Add <-> to GiST box_ops.
Extracted gist_box_distance_helper() common for gist_box_distance() and
gist_bbox_distance().
For me it doesn't look worth having two distinct functions
gist_box_distance_helper() and gist_bbox_distance(). What about
having just one and leave responsibility for recheck flag to the
caller?
gist_bbox_distance() was removed.
But maybe it would be better to replace two identical functions
gist_circle_distance() and gist_poly_distance() with the single
gist_bbox_distance()?
3. Add <-> to SP-GiST.
Changed only catalog and tests. Box case is already checked in
spg_box_quad_leaf_consistent():
out->recheckDistances = distfnoid == F_DIST_POLYP;
So, it seems to be fix of oversight in 2a6368343ff4. But assuming
fixing this requires catalog changes, we shouldn't backpatch this.
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
>From 14a0e22e1b68b084dd75a7f660955b52b6aaae79 Mon Sep 17 00:00:00 2001
From: Nikita Glukhov
Date: Mon, 8 Jul 2019 22:55:00 +0300
Subject: [PATCH 1/3] Add missing distance operators
---
src/backend/utils/adt/geo_ops.c| 136 -
src/include/catalog/pg_operator.dat| 42 +-
src/include/catalog/pg_proc.dat| 25 +
src/test/regress/expected/geometry.out | 986 +
src/test/regress/sql/geometry.sql | 15 +-
5 files changed, 687 insertions(+), 517 deletions(-)
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index 373784f..6558ea3 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -2348,6 +2348,17 @@ dist_pl(PG_FUNCTION_ARGS)
PG_RETURN_FLOAT8(line_closept_point(NULL, line, pt));
}
+/*
+ * Distance from a line to a point
+ */
+Datum
+dist_lp(PG_FUNCTION_ARGS)
+{
+ LINE *line = PG_GETARG_LINE_P(0);
+ Point *pt = PG_GETARG_POINT_P(1);
+
+ PG_RETURN_FLOAT8(line_closept_point(NULL, line, pt));
+}
/*
* Distance from a point to a lseg
@@ -2362,13 +2373,20 @@ dist_ps(PG_FUNCTION_ARGS)
}
/*
- * Distance from a point to a path
+ * Distance from a lseg to a point
*/
Datum
-dist_ppath(PG_FUNCTION_ARGS)
+dist_sp(PG_FUNCTION_ARGS)
+{
+ LSEG *lseg = PG_GETARG_LSEG_P(0);
+ Point *pt = PG_GETARG_POINT_P(1);
+
+ PG_RETURN_FLOAT8(lseg_closept_point(NULL, lseg, pt));
+}
+
+static float8
+dist_ppath_internal(Point *pt, PATH *path)
{
- Point *pt = PG_GETARG_POINT_P(0);
- PATH *path = PG_GETARG_PATH_P(1);
float8 result = 0.0; /* keep compiler quiet */
bool have_min = false;
float8 tmp;
@@ -2403,7 +2421,31 @@ dist_ppath(PG_FUNCTION_ARGS)
}
}
- PG_RETURN_FLOAT8(result);
+ return result;
+}
+
+/*
+ * Distance from a point to a path
+ */
+Datum
+dist_ppath(PG_FUNCTION_ARGS)
+{
+ Point *pt = PG_GETARG_POINT_P(0);
+ PATH *path = PG_GETARG_PATH_P(1);
+
+ PG_RETURN_FLOAT8(dist_ppath_internal(pt, path));
+}
+
+/*
+ * Distance from a path to a point
+ */
+Datum
+dist_pathp(PG_FUNCTION_ARGS)
+{
+ PATH *path = PG_GETARG_PATH_P(0);
+ Point *pt = PG_GETARG_POINT_P(1);
+
+ PG_RETURN_FLOAT8(dist_ppath_internal(pt, path));
}
/*
@@ -2419,6 +2461,18 @@ dist_pb(PG_FUNCTION_ARGS)
}
/*
+ * Distance from a box to a point
+ */
+Datum
+dist_bp(PG_FUNCTION_ARGS)
+{
+ BOX *box = PG_GETARG_BOX_P(0);
+ Point *pt = PG_GETARG_POINT_P(1);
+
+ PG_RETURN_FLOAT8(box_closept_point(NULL, box, pt));
+}
+
+/*
* Distance from a lseg to a line
*/
Datum
@@ -2431,6 +2485,18 @@ dist_sl(PG_FUNCTION_ARGS)
}
/*
+ * Distance from a line to a lseg
+ */
+Datum
+dist_ls(PG_FUNCTION_ARGS)
+{
+ LINE *line = PG_GETARG_LINE_P(0);
+ LSEG *lseg = PG_GETARG_LSEG_P(1);
+
+ PG_RETURN_FLOAT8(lseg_closept_line(NULL, lseg, line));
+}
+
+/*
* Distance from a lseg to a box
*/
Datum
@@ -2443,6 +2509,18 @@ dist_sb(PG_FUNCTION_ARGS)
}
/*
+ * Distance from a box to a lseg
+ */
+Datum
+dist_bs(PG_FUNCTION_ARGS)
+{
+ BOX *box = PG_GETARG_BOX_P(0);
+ LSEG *lseg = PG_GETARG_LSEG_P(1);
+
+ PG_RETURN_FLOAT8(box_closept_lseg(NULL, box, lseg));
+}
+
+/*
* Distance from a line to a box
*/
Datum
@@ -2462,13
Re: Add missing operator <->(box, point)
On Mon, Mar 11, 2019 at 2:49 AM Nikita Glukhov wrote: > 2. Add <-> to GiST box_ops. >Extracted gist_box_distance_helper() common for gist_box_distance() and >gist_bbox_distance(). For me it doesn't look worth having two distinct functions gist_box_distance_helper() and gist_bbox_distance(). What about having just one and leave responsibility for recheck flag to the caller? > 3. Add <-> to SP-GiST. >Changed only catalog and tests. Box case is already checked in >spg_box_quad_leaf_consistent(): > out->recheckDistances = distfnoid == F_DIST_POLYP; So, it seems to be fix of oversight in 2a6368343ff4. But assuming fixing this requires catalog changes, we shouldn't backpatch this. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Add missing operator <->(box, point)
Alexander Korotkov writes: > On Tue, Jul 2, 2019 at 9:19 PM Nikita Glukhov wrote: >> We could use commuted "const <-> var" operators for kNN searches, but the >> current implementation requires the existence of "var <-> const" operators, >> and >> order-by-op clauses are rebuilt using them (see match_clause_to_ordering_op() >> at /src/backend/optimizer/path/indxpath.c). > But probably it's still worth to just add commutator for every <-> > operator and close this question. Yeah, I was just thinking that it was weird not to have the commutator operators, independently of indexing considerations. Seems like a usability thing. regards, tom lane
Re: Add missing operator <->(box, point)
On Tue, Jul 2, 2019 at 9:19 PM Nikita Glukhov wrote: > We could use commuted "const <-> var" operators for kNN searches, but the > current implementation requires the existence of "var <-> const" operators, > and > order-by-op clauses are rebuilt using them (see match_clause_to_ordering_op() > at /src/backend/optimizer/path/indxpath.c). But probably it's still worth to just add commutator for every <-> operator and close this question. Otherwise, it may arise again once we want to add some more kNN support to opclasses or something. On the other hand, are we already going to limit oid consumption? -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Add missing operator <->(box, point)
Attached 2nd version of the patches.
On 20.04.2019 16:41, Fabien COELHO wrote:
About the test, I'd suggest to name the result columns, eg "pt to box
dist" and "box to pt dist", otherwise why all is repeated is unclear.
Fixed.
On 02.07.2019 7:01, Tom Lane wrote:
[ warning, drive-by comment ahead ]
Fabien COELHO writes:
I notice that other distance tests do not test for commutativity. Are they
also not implemented, or just not tested? If not implemented, I'd suggest
to add them in the same batch.
Yeah ... just looking at operators named <->, I see
regression=# select oid, oid::regoperator, oprcom, oprcode from pg_operator where
oprname = '<->';
oid | oid | oprcom | oprcode
--+--++---
517 | <->(point,point) |517 | point_distance
613 | <->(point,line) | 0 | dist_pl
614 | <->(point,lseg) | 0 | dist_ps
615 | <->(point,box) | 0 | dist_pb
616 | <->(lseg,line) | 0 | dist_sl
617 | <->(lseg,box)| 0 | dist_sb
618 | <->(point,path) | 0 | dist_ppath
706 | <->(box,box) |706 | box_distance
707 | <->(path,path) |707 | path_distance
708 | <->(line,line) |708 | line_distance
709 | <->(lseg,lseg) |709 | lseg_distance
712 | <->(polygon,polygon) |712 | poly_distance
1520 | <->(circle,circle) | 1520 | circle_distance
1522 | <->(point,circle)| 3291 | dist_pc
3291 | <->(circle,point)| 1522 | dist_cpoint
3276 | <->(point,polygon) | 3289 | dist_ppoly
3289 | <->(polygon,point) | 3276 | dist_polyp
1523 | <->(circle,polygon) | 0 | dist_cpoly
1524 | <->(line,box)| 0 | dist_lb
5005 | <->(tsquery,tsquery) | 0 | pg_catalog.tsquery_phrase
(20 rows)
It's not clear to me why to be particularly more excited about
<->(box, point) than about the other missing cases here.
regards, tom lane
The original goal was to add support of ordering by distance to point to
all geometric opclasses. As you can see, GiST and SP-GiST box_ops has no
distance operator while more complex circle_ops and poly_ops have it:
SELECT
amname,
opcname,
amopopr::regoperator AS dist_opr
FROM
pg_opclass LEFT JOIN
pg_amop ON amopfamily = opcfamily AND amoppurpose = 'o',
pg_am,
pg_type
WHERE
opcmethod = pg_am.oid AND
opcintype = pg_type.oid AND
typcategory = 'G'
ORDER BY 1, 2;
amname | opcname | dist_opr
+---+
brin | box_inclusion_ops |
gist | box_ops |
gist | circle_ops| <->(circle,point)
gist | point_ops | <->(point,point)
gist | poly_ops | <->(polygon,point)
spgist | box_ops |
spgist | kd_point_ops | <->(point,point)
spgist | poly_ops | <->(polygon,point)
spgist | quad_point_ops| <->(point,point)
(9 rows)
We could use commuted "const <-> var" operators for kNN searches, but the
current implementation requires the existence of "var <-> const" operators, and
order-by-op clauses are rebuilt using them (see match_clause_to_ordering_op()
at /src/backend/optimizer/path/indxpath.c).
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
>From cff9fbdcd9d3633a28665c0636e6d3a5a0e8512b Mon Sep 17 00:00:00 2001
From: Nikita Glukhov
Date: Thu, 7 Mar 2019 20:22:49 +0300
Subject: [PATCH 1/3] Add operator <->(box, point)
---
src/backend/utils/adt/geo_ops.c| 12 +
src/include/catalog/pg_operator.dat| 5 +-
src/include/catalog/pg_proc.dat| 3 ++
src/test/regress/expected/geometry.out | 96 +-
src/test/regress/sql/geometry.sql | 2 +-
5 files changed, 68 insertions(+), 50 deletions(-)
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index 373784f..d8ecfe3 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -2419,6 +2419,18 @@ dist_pb(PG_FUNCTION_ARGS)
}
/*
+ * Distance from a box to a point
+ */
+Datum
+dist_bp(PG_FUNCTION_ARGS)
+{
+ BOX *box = PG_GETARG_BOX_P(0);
+ Point *pt = PG_GETARG_POINT_P(1);
+
+ PG_RETURN_FLOAT8(box_closept_point(NULL, box, pt));
+}
+
+/*
* Distance from a lseg to a line
*/
Datum
diff --git a/src/include/catalog/pg_operator.dat b/src/include/catalog/pg_operator.dat
index bacafa5..ae5ed1e 100644
--- a/src/include/catalog/pg_operator.dat
+++ b/src/include/catalog/pg_operator.dat
@@ -666,7 +666,10 @@
oprresult => 'float8', oprcode => 'dist_ps' },
{ oid => '615', descr => 'distance between',
oprname => '<->', oprleft => 'point', oprright => 'box',
- oprresult => 'float8', oprcode => 'dist_pb' },
+ oprresult => 'float8', oprcom => '<->(box,point)', oprcode => 'dist_pb' },
+{ oid => '606', descr => 'distance between',
+ oprname => '<->', oprleft => 'box', oprright =>
Re: Add missing operator <->(box, point)
[ warning, drive-by comment ahead ] Fabien COELHO writes: > I notice that other distance tests do not test for commutativity. Are they > also not implemented, or just not tested? If not implemented, I'd suggest > to add them in the same batch. Yeah ... just looking at operators named <->, I see regression=# select oid, oid::regoperator, oprcom, oprcode from pg_operator where oprname = '<->'; oid | oid | oprcom | oprcode --+--++--- 517 | <->(point,point) |517 | point_distance 613 | <->(point,line) | 0 | dist_pl 614 | <->(point,lseg) | 0 | dist_ps 615 | <->(point,box) | 0 | dist_pb 616 | <->(lseg,line) | 0 | dist_sl 617 | <->(lseg,box)| 0 | dist_sb 618 | <->(point,path) | 0 | dist_ppath 706 | <->(box,box) |706 | box_distance 707 | <->(path,path) |707 | path_distance 708 | <->(line,line) |708 | line_distance 709 | <->(lseg,lseg) |709 | lseg_distance 712 | <->(polygon,polygon) |712 | poly_distance 1520 | <->(circle,circle) | 1520 | circle_distance 1522 | <->(point,circle)| 3291 | dist_pc 3291 | <->(circle,point)| 1522 | dist_cpoint 3276 | <->(point,polygon) | 3289 | dist_ppoly 3289 | <->(polygon,point) | 3276 | dist_polyp 1523 | <->(circle,polygon) | 0 | dist_cpoly 1524 | <->(line,box)| 0 | dist_lb 5005 | <->(tsquery,tsquery) | 0 | pg_catalog.tsquery_phrase (20 rows) It's not clear to me why to be particularly more excited about <->(box, point) than about the other missing cases here. regards, tom lane
Re: Add missing operator <->(box, point)
Hello Nikita, Attached patches add missing distance operator <->(box, point). Indeed. We already have reverse operator <->(point, box), but it can't be used for kNN search in GiST and SP-GiST. GiST and SP-GiST now support kNN searches over more complex polygons and circles, but do not support more simple boxes, which seems to be inconsistent. Description of the patches: 1. Add function dist_pb(box, point) and operator <->. About this first patch: applies cleanly, compiles, "make check" ok. No doc changes, but this was expected to work in the first place, according to the documention. About the test, I'd suggest to name the result columns, eg "pt to box dist" and "box to pt dist", otherwise why all is repeated is unclear. I notice that other distance tests do not test for commutativity. Are they also not implemented, or just not tested? If not implemented, I'd suggest to add them in the same batch. If not tested, maybe the patch should do as others, or maybe given the trivial implementation there should just be one test per commutted operator for coverage. ISTM that the committer would need to "bump the catalog revision number" because it adds new functions & operators. -- Fabien.
