- Notifications
You must be signed in to change notification settings - Fork15
KNN support for spoints#116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
doc/indices.sgm Outdated
| @@ -68,6 +68,11 @@ | |||
| </para> | |||
| </listitem> | |||
| </itemizedlist> | |||
| <para> | |||
| GiST index can be used also for fast finding points closest to the given one | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Change "fast finding points" to "quickly finding the points".
doc/indices.sgm Outdated
| <para> | ||
| GiST index can be used also for fast finding points closest to the given one | ||
| when ordering by an expression with the <literal><-></literal> operator | ||
| is used, as shown in an example below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove "is used" and move the comma to be at the end of "operator" on the previous line.
doc/indices.sgm Outdated
| @@ -100,7 +112,6 @@ | |||
| <![CDATA[CREATE INDEX test_pos_idx USING BRIN ON test (pos) WITH (pages_per_range = 16);]]> | |||
| </programlisting> | |||
| </example> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Based on the other</sect1> tags I saw, I don't think this blank line before</sect1> should have been removed.
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Create an upgrade script and add this function to it.
pgs_gist.sql.in Outdated
| @@ -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, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Whitespace doesn't match the otherOPERATOR lines surrounding this addition. The17 should line up with the16 on the previous line for starters.
src/gist.c Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove the space afteracos.
src/gist.c Outdated
| 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changeif( toif ( throughout this function in order to match the coding style of pgsphere.
src/gist.c Outdated
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changedistance_vector_point_3d ( todistance_vector_point_3d( throughout this function in order to match the coding style of pgsphere.
| @@ -1672,6 +1677,127 @@ fallbackSplit(Box3D *boxes, OffsetNumber maxoff, GIST_SPLITVEC *v) | |||
| v->spl_ldatum_exists = v->spl_rdatum_exists = false; | |||
| } | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
An extra blank line doesn't need to be added here.
src/gist.c Outdated
| PG_RETURN_FLOAT8(retval); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Two too many blank lines added here, I think.
I used pgindent to reformat new functions in gist.c. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A couple more very minor changes to the documentation, please.
Still needs an upgrade script. And maybe a version bump?
doc/indices.sgm Outdated
| @@ -68,6 +68,11 @@ | |||
| </para> | |||
| </listitem> | |||
| </itemizedlist> | |||
| <para> | |||
| GiST index can be also used for quickly finding the points closest to the given one | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ChangeGiST index toA GiST index. Sorry should have caught that in my previous review.
doc/indices.sgm Outdated
| @@ -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><-></literal> operator: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ChangeTo find points toTo find the points.
| @@ -0,0 +1,16 @@ | |||
| -- Upgrade: 1.4.2 -> 1.5.0 | |||
| CREATE FUNCTION g_spoint_distance(internal, spoint, smallint, oid, internal) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
UseCREATE OR REPLACE in the upgrade script? See issue#111.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me!
Raise the version up to 1.5.0, add upgrade scriptFix issues found after the review
Squashed latest 3 commits. The two commits are kept - the original commit and the commit with code adjustment. |
This is the original patch for review and improvement.