geo/geomfn: implement ST_LineInterpolatePoint(s) #49742

abhishek20123g · 2020-05-31T19:51:43Z

This PR adds following builtin functions

ST_LineInterpolatePoint{{geometry, float8}}
ST_LineInterpolatePoints{{geometry, float8, bool}}

which works for LineString only, allows us to determine one or more
interpolated points in the LineString which at an integral multiple
of given fraction of LineString's total length.

Release note (sql change): This PR implement adds the following built-in functions.

ST_LineInterpolatePoint{{geometry, float8}}
ST_LineInterpolatePoints{{geometry, float8, bool}}

blathers-crl · 2020-05-31T19:51:48Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

@otan (author of geo/geomfn: implement ST_LineInterpolatePoint({geometry,float8}) #48971, author of geo/geomfn: implement ST_LineInterpolatePoints({geometry,float8,bool}) #48972)

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

cockroach-teamcity · 2020-05-31T19:51:49Z

This change is

abhishek20123g · 2020-05-31T20:18:08Z

Small Overview:

Thought these functions can't be completely called as unary functions, but I had kept them in unary_operators.go file, as it may be appropriate place for them and, I am not completely sure of creating a new file for them is a good idea. In Geos they are kept under separate location named Linear referencing functions
https://github.com/libgeos/geos/blob/master/capi/geos_c.h.in#L359.
logictest can be added without creating table geom_linear but I think geom_linear table may be helpful for other linear built-in functions.
Error messages are not same as they are in PostGIS I had changed them a little If they are not appropriate I can make them same as PostGIS
the error message is postGIS are :

gis=# select st_lineinterpolatepoint('linestring(0 0, 1 1)'::geometry, -1);
ERROR:  line_interpolate_point: 2nd arg isn't within [0,1]

gis=# select st_lineinterpolatepoint('multilinestring((0 0, 1 1))'::geometry, 0.2);
ERROR:  line_interpolate_point: 1st arg isn't a line

doc statements are also slightly deviated from PostGIS doc.
In PostGIS they are as:

Returns a point interpolated along a line. First argument must be a LINESTRING. Second argument is a float8 between 0 and 1 representing fraction of total linestring length the point has to be located.

Returns one or more points interpolated along a line. First argument must be a LINESTRING. Second argument is a float8 between 0 and 1 representing the spacing between the points as a fraction of total LineString length. If the third argument is false, at most one point will be constructed (the function will be equivalent to ST_LineInterpolatePoint.)

If the result has zero or one points, it will be returned as a POINT. If it has two or more points, it will be returned as a MULTIPOINT.

blathers-crl · 2020-05-31T20:33:22Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

otan

glad to see you back! looking good, mostly minor nits.

nit: title does not fit in PR message. can you make it:
geo/geomfn: implement ST_LineInterpolatePoint(s)

otan · 2020-05-31T22:05:14Z

pkg/sql/sem/builtins/geo_builtins.go

+ },
+ Info: infoBuilder{
+ info: builtinInfo,
+ libraryUsage: usesGEOS,


this function does not use an index; please remove canUseIdex: true,

otan · 2020-05-31T22:05:20Z

pkg/sql/sem/builtins/geo_builtins.go

+
+Note If the result has zero or one points, it will be returned as a POINT. If it has two or more points, it will be returned as a MULTIPOINT.`,
+ libraryUsage: usesGEOS,
+ canUseIndex: true,


this function does not use an index; please remove canUseIdex: true,

done for all places.

otan · 2020-05-31T22:30:13Z

pkg/geo/geos/geos.cc

@@ -119,6 +120,7 @@ struct CR_GEOS {
 CR_GEOS_Area_r GEOSArea_r;
 CR_GEOS_Length_r GEOSLength_r;
 CR_GEOS_Centroid_r GEOSGetCentroid_r;
+ CR_GEOS_Interpolate_r GEOSInterpolateNormalized_r;


can you explain why we're using InterpolateNormalized? if this is required, please rename Interpolate_r to InterpolateNormalized_r.

Changed Geos function which we're using for interpolation. Previously it was GEOSInterpolateNormalized now it is GEOSInterpolate and Updated their names according to GEOSInterpolate.

Initially, I was using GEOSInterpolateNormalized as it was directly satisfying the use-case as it works with fraction directly.
But later realised that It is wrapper function over GEOSInterpolate inside which we are computing LineString's length each time it called.
Which could be inefficient for MULTIPOINT case as It can be done just computing length only once.
And there may be the case in future where we may require to locate a point at a certain distance. For which GEOSInterpolate will be the best option rather somehow using GEOSInterpolateNormalized

otan · 2020-05-31T22:30:50Z

pkg/sql/logictest/testdata/logic_test/geospatial

@@ -1428,3 +1428,72 @@ ORDER BY id
 1 4004 0
 2 4326 4326
 3 4004 3857
+


subtest geom_linear, maybe?

otan · 2020-05-31T22:32:06Z

pkg/sql/sem/builtins/geo_builtins.go

+ defProps(),
+ lineInterpolatePointForRepeatOverload(
+ false,
+ `Returns point along the given LineString which is at given fraction of LineString's total length.`,


nit: returns a point

otan · 2020-05-31T22:32:33Z

pkg/sql/logictest/testdata/logic_test/geospatial

+INSERT INTO geom_linear VALUES
+ ('Empty LineString', 'LINESTRING EMPTY'),
+ ('LineString anticlockwise covering all the quadrants', 'LINESTRING(1 -1, 2 2, -2 2, -1 -1)'),
+ ('LineString clockwise covering all the quadrants with SRID 4004', 'SRID=4004;LINESTRING(1 -1, -1 -1, -2 2, 2 2)')


can you add tests to make sure this behaves as appropriate for POLYGON/POINT/MULTILINESTRING?

Since POLYGON/POINT/MULTILINESTRING each of them will produce an error, therefore added separately query for them at the bottom.
Currently, I am unable to figure out how could we add test cases for them by inserting elements in the table.

Similar to this added test case for POLYGON/POINT/MULTILINESTRING,
in unary_operator_test.go too.

otan · 2020-05-31T22:33:50Z

pkg/geo/geomfn/unary_operators_test.go

+ }
+ for _, test := range testCasesForLineInterpolate {
+ for _, repeat := range []bool{false, true} {
+ t.Run(fmt.Sprintf("%s for fraction %v where repeat is %t", test.wkb, test.fraction, repeat),


nit: fraction %f

Changed!
Along with this error message are also changes. As were also using %v for float in error message too.

otan · 2020-06-01T17:05:18Z

bors r+

craig · 2020-06-01T17:20:02Z

Merge conflict (retrying...)

craig · 2020-06-01T17:20:14Z

Merge conflict

otan · 2020-06-01T18:08:52Z

fixing the conflict for you, moving the file to linear_reference.go

abhishek20123g · 2020-06-01T18:18:47Z

Thank you for the review 😊

Fixes cockroachdb#48971 Fixes cockroachdb#48972 This PR adds following builtin functions * ST_LineInterpolatePoint{{geometry, float8}} * ST_LineInterpolatePoints{{geometry, float8, bool}} which works for LineString only, allows us to determine one or more interpolated points in the LineString which at an integral multiple of given fraction of LineString's total length. Release note (sql change): This PR implement adds the following built-in functions. * ST_LineInterpolatePoint{{geometry, float8}} * ST_LineInterpolatePoints{{geometry, float8, bool}}

otan · 2020-06-02T05:32:05Z

bors r+

craig · 2020-06-02T06:07:56Z

Build succeeded

GitHub CI (Cockroach)

blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels May 31, 2020

blathers-crl bot requested a review from otan May 31, 2020 19:51

miretskiy mentioned this pull request May 31, 2020

cdc: Treat node draining errors as retryable. #49743

Merged

abhishek20123g force-pushed the geom-interpolations branch from 8af28d1 to 4d35e3c Compare May 31, 2020 20:33

otan reviewed May 31, 2020

View reviewed changes

abhishek20123g force-pushed the geom-interpolations branch from 4d35e3c to 7c0c7f8 Compare June 1, 2020 12:07

blathers-crl bot requested a review from otan June 1, 2020 12:07

abhishek20123g changed the title ~~builtin: Implements ST_LineInterpolatePoint and ST_LineInterpolatePoi…~~ geo/geomfn: implement ST_LineInterpolatePoint(s) Jun 1, 2020

otan approved these changes Jun 1, 2020

View reviewed changes

otan force-pushed the geom-interpolations branch from 7c0c7f8 to 7be47bc Compare June 1, 2020 18:10

otan force-pushed the geom-interpolations branch from 7be47bc to cf81e9f Compare June 2, 2020 05:32

craig bot merged commit cbe7fdc into cockroachdb:master Jun 2, 2020

jseldess mentioned this pull request Jun 24, 2020

geo/geomfn: implement ST_LineInterpolatePoint(s) cockroachdb/docs#7588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

geo/geomfn: implement ST_LineInterpolatePoint(s) #49742

geo/geomfn: implement ST_LineInterpolatePoint(s) #49742

abhishek20123g commented May 31, 2020 •

edited

Loading

blathers-crl bot commented May 31, 2020

cockroach-teamcity commented May 31, 2020

abhishek20123g commented May 31, 2020

blathers-crl bot commented May 31, 2020

otan left a comment

otan May 31, 2020

otan May 31, 2020

abhishek20123g Jun 1, 2020

otan May 31, 2020

abhishek20123g Jun 1, 2020

otan May 31, 2020

abhishek20123g Jun 1, 2020

otan May 31, 2020

otan May 31, 2020

abhishek20123g Jun 1, 2020

otan May 31, 2020

abhishek20123g Jun 1, 2020

otan commented Jun 1, 2020

craig bot commented Jun 1, 2020

craig bot commented Jun 1, 2020

otan commented Jun 1, 2020

abhishek20123g commented Jun 1, 2020

otan commented Jun 2, 2020

craig bot commented Jun 2, 2020

geo/geomfn: implement ST_LineInterpolatePoint(s) #49742

geo/geomfn: implement ST_LineInterpolatePoint(s) #49742

Conversation

abhishek20123g commented May 31, 2020 • edited Loading

blathers-crl bot commented May 31, 2020

cockroach-teamcity commented May 31, 2020

abhishek20123g commented May 31, 2020

blathers-crl bot commented May 31, 2020

otan 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

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

Choose a reason for hiding this comment

otan commented Jun 1, 2020

craig bot commented Jun 1, 2020

Merge conflict (retrying...)

craig bot commented Jun 1, 2020

Merge conflict

otan commented Jun 1, 2020

abhishek20123g commented Jun 1, 2020

otan commented Jun 2, 2020

craig bot commented Jun 2, 2020

Build succeeded

abhishek20123g commented May 31, 2020 •

edited

Loading