KNN support for spoints #116

vitcpp · 2023-12-12T05:00:30Z

This is the original patch for review and improvement.

esabol · 2023-12-12T06:29:48Z

doc/indices.sgm

@@ -68,6 +68,11 @@
            </para>
          </listitem>
        </itemizedlist>
+        <para>
+          GiST index can be used also for fast finding points closest to the given one 


Change "fast finding points" to "quickly finding the points".

esabol · 2023-12-12T06:31:02Z

doc/indices.sgm

+        <para>
+          GiST index can be used also for fast finding points closest to the given one 
+          when ordering by an expression with the <literal>&lt;-&gt;</literal> operator
+          is used, as shown in an example below.


Remove "is used" and move the comma to be at the end of "operator" on the previous line.

esabol · 2023-12-12T06:32:43Z

doc/indices.sgm

@@ -100,7 +112,6 @@
 <![CDATA[CREATE INDEX test_pos_idx USING BRIN ON test (pos) WITH (pages_per_range = 16);]]>
          </programlisting>
        </example>
-


Based on the other </sect1> tags I saw, I don't think this blank line before </sect1> should have been removed.

esabol · 2023-12-12T06:34:06Z

pgs_gist.sql.in

 CREATE FUNCTION g_spoint_consistent(internal, internal, int4, oid, internal)
   RETURNS internal
   AS 'MODULE_PATHNAME', 'g_spoint_consistent'
   LANGUAGE 'c';

+CREATE FUNCTION g_spoint_distance(internal, spoint, smallint, oid, internal)


Create an upgrade script and add this function to it.

esabol · 2023-12-12T06:36:03Z

pgs_gist.sql.in

@@ -114,6 +117,7 @@ CREATE OPERATOR CLASS spoint
   OPERATOR  14 @ (spoint, spoly),
   OPERATOR  15 @ (spoint, sellipse),
   OPERATOR  16 @ (spoint, sbox),
+   OPERATOR	 17	<-> (spoint, spoint) FOR ORDER BY float_ops,


Whitespace doesn't match the other OPERATOR lines surrounding this addition. The 17 should line up with the 16 on the previous line for starters.

esabol · 2023-12-12T06:42:13Z

src/gist.c

@@ -681,6 +682,10 @@ g_spoint3_consistent(PG_FUNCTION_ARGS)
 	PG_RETURN_BOOL(false);
 }

+static double distance_vector_point_3d (Vector3D* v, double x, double y, double z) { 
+	return acos ( (v->x * x + v->y * y + v->z * z) / sqrt( x*x + y*y + z*z ) ); //  as v has length=1 by design


Remove the space after acos.

esabol · 2023-12-12T06:44:33Z

src/gist.c

+			v_high.y = (double)box->high.coord[1]  / MAXCVALUE;
+			v_high.z = (double)box->high.coord[2]  / MAXCVALUE;
+			//  a box splits space into 27 subspaces (6+12+8+1) with different distance calculation
+			if(v_point.x < v_low.x) { 


Change if( to if ( throughout this function in order to match the coding style of pgsphere.

esabol · 2023-12-12T06:46:58Z

src/gist.c

+			if(v_point.x < v_low.x) { 
+				if(v_point.y < v_low.y) { 
+					if(v_point.z < v_low.z) { 
+						retval = distance_vector_point_3d (&v_point, v_low.x, v_low.y, v_low.z); //point2point distance


Change distance_vector_point_3d ( to distance_vector_point_3d( throughout this function in order to match the coding style of pgsphere.

esabol · 2023-12-12T06:48:03Z

src/gist.c

@@ -1672,6 +1677,127 @@ fallbackSplit(Box3D *boxes, OffsetNumber maxoff, GIST_SPLITVEC *v)
 	v->spl_ldatum_exists = v->spl_rdatum_exists = false;
 }

+


An extra blank line doesn't need to be added here.

esabol · 2023-12-12T06:48:52Z

src/gist.c

+	PG_RETURN_FLOAT8(retval);
+}
+
+


Two too many blank lines added here, I think.

vitcpp · 2023-12-12T15:02:56Z

I used pgindent to reformat new functions in gist.c.

esabol

A couple more very minor changes to the documentation, please.

Still needs an upgrade script. And maybe a version bump?

esabol · 2023-12-12T15:44:36Z

doc/indices.sgm

@@ -68,6 +68,11 @@
            </para>
          </listitem>
        </itemizedlist>
+        <para>
+          GiST index can be also used for quickly finding the points closest to the given one


Change GiST index to A GiST index. Sorry should have caught that in my previous review.

esabol · 2023-12-12T15:45:41Z

doc/indices.sgm

@@ -82,6 +87,13 @@
 <![CDATA[CREATE INDEX test_pos_idx ON test USING GIST (pos);]]>
 <![CDATA[VACUUM ANALYZE test;]]>
          </programlisting>
+          <para>
+           To find points closest to a given spherical position, use the <literal>&lt;-&gt;</literal> operator:


Change To find points to To find the points.

esabol · 2023-12-17T17:34:16Z

upgrade_scripts/pg_sphere--1.4.2--1.5.0.sql.in

@@ -0,0 +1,16 @@
+-- Upgrade: 1.4.2 -> 1.5.0
+
+CREATE FUNCTION g_spoint_distance(internal, spoint, smallint, oid, internal)


Use CREATE OR REPLACE in the upgrade script? See issue #111.

esabol

Looks good to me!

Raise the version up to 1.5.0, add upgrade script Fix issues found after the review

vitcpp · 2023-12-20T12:00:30Z

Squashed latest 3 commits. The two commits are kept - the original commit and the commit with code adjustment.

KNN support for spoints

b7152a2

esabol suggested changes Dec 12, 2023

View reviewed changes

vitcpp force-pushed the knn branch from c772c92 to ba24afb Compare December 12, 2023 13:49

esabol suggested changes Dec 12, 2023

View reviewed changes

esabol suggested changes Dec 17, 2023

View reviewed changes

esabol approved these changes Dec 19, 2023

View reviewed changes

Fix code formatting, bump release version up to 1.5.0

77401cb

Raise the version up to 1.5.0, add upgrade script Fix issues found after the review

vitcpp force-pushed the knn branch from d94dceb to 77401cb Compare December 20, 2023 11:53

vitcpp merged commit f20a664 into postgrespro:master Dec 22, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KNN support for spoints #116

KNN support for spoints #116

vitcpp commented Dec 12, 2023

esabol Dec 12, 2023

esabol Dec 12, 2023

esabol Dec 12, 2023 •

edited

Loading

esabol Dec 12, 2023

esabol Dec 12, 2023

esabol Dec 12, 2023

esabol Dec 12, 2023

esabol Dec 12, 2023

esabol Dec 12, 2023

esabol Dec 12, 2023

vitcpp commented Dec 12, 2023

esabol left a comment

esabol Dec 12, 2023

esabol Dec 12, 2023

esabol Dec 17, 2023

esabol left a comment

vitcpp commented Dec 20, 2023

		@@ -1672,6 +1677,127 @@ fallbackSplit(Box3D boxes, OffsetNumber maxoff, GIST_SPLITVEC v)
		v->spl_ldatum_exists = v->spl_rdatum_exists = false;
		}

		@@ -0,0 +1,16 @@
		-- Upgrade: 1.4.2 -> 1.5.0

		CREATE FUNCTION g_spoint_distance(internal, spoint, smallint, oid, internal)

KNN support for spoints #116

KNN support for spoints #116

Conversation

vitcpp commented Dec 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esabol Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitcpp commented Dec 12, 2023

esabol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esabol left a comment

Choose a reason for hiding this comment

vitcpp commented Dec 20, 2023

esabol Dec 12, 2023 •

edited

Loading