builtin: Implements ST_Segmentize builtin function #49211

abhishek20123g · 2020-05-18T18:54:59Z

This PR adds ST_Segmentize({geography,float8}) builtin function
which allows modify given geography such that no segment longer
than the given max_segment_length.

Release note (sql change): This PR implements ST_Segmentize
builtin functions.

blathers-crl · 2020-05-18T18:55:03Z

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/geogfn: implement ST_Segmentize({geography,float8}) #48368)

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

cockroach-teamcity · 2020-05-18T18:55:07Z

This change is

abhishek20123g · 2020-05-18T19:07:06Z

Small Overview:

Not sure its good idea to create the separate file as geog_modification.go
as all Other files are not suiting the definition of ST_Segmentize.
SegmentizeGeography and segmentizeGeom can be clubbed together but it was
easy to use recursion if they are separate.
geog_modification_test.go follow the same test case and structure as was in
unary_operator_test.go.
Since I am new to S2 and geom library, tried to find a suitable function for interpolation or segmentization in geom library but unable to find suitable therefore it has to cast to s2.point as s2 library consist function for interpolation.
Not sure the usage of canUseIndex in infoBuilder therefore set it to true as most of the relatable builtin have it as true.

otan

Thank you for the contribution - this was harder than I expected and thanks for perservering!

I have a few comments that are mostly style related.

pkg/geo/geogfn/geog_modification.go

pkg/geo/geo.go

pkg/geo/geogfn/geog_modification.go

otan · 2020-05-18T20:44:03Z

pkg/geo/geogfn/geog_modification.go

+	case *geom.MultiLineString:
+		segMultiLine := geom.NewMultiLineString(geom.XY).SetSRID(geometry.SRID())
+		for lineIdx := 0; lineIdx < geometry.NumLineStrings(); lineIdx++ {
+			l, _ := segmentizeGeom(geometry.LineString(lineIdx), segmentMaxLength)


ignoring this error seems bad. please do the if err != nil { ... } here.

Updated,
I just left error unhandled as *geom.LinearRing and *geom.LineString will never return.

pkg/geo/geogfn/geog_modification.go

pkg/geo/geogfn/geog_modification_test.go

otan · 2020-05-18T21:00:35Z

pkg/sql/sem/builtins/geo_builtins.go

+			Info: infoBuilder{
+				info: "Returns a modified geometry having no segment longer than the given max_segment_length. " +
+					"Distance computation is performed in 2d only. Units are in meters.",
+				libraryUsage: usesGeographicLib | usesS2,


we don't actually use GeographicLib here, so just libraryUsage: usesS2 is fine.

otan · 2020-05-18T21:01:17Z

pkg/sql/sem/builtins/geo_builtins.go

+				return tree.NewDGeography(segGeometry), nil
+			},
+			Info: infoBuilder{
+				info: "Returns a modified geometry having no segment longer than the given max_segment_length. " +


Returns a modified Geography having no segment longer than the given max_segment_length meters. +
The calculations are done on a sphere.

otan · 2020-05-18T21:02:30Z

pkg/geo/geogfn/geog_modification_test.go

+		geogWkt: "LINESTRING(1.0 1.0, 2.0 2.0, 3.0 3.0)",
+		segmentize: segmentizedMaxLengthAndExpGeog{
+			maxSegmentLength: 100000.0,
+			segmentizedGeogWkt: "LINESTRING (1 1, 1.4998857365616758 1.5000570914791973, 2 2, 2.4998094835255658 2.500095075163195, 3 3)",


can we add tests for no segmentization needed for LineString / Polygon?

otan · 2020-05-18T21:03:28Z

pkg/geo/geogfn/geog_modification.go

@@ -0,0 +1,145 @@
+// Copyright 2020 The Cockroach Authors.


nit also: you can name this segmentize.go / segmentize_test.go.

blathers-crl · 2020-05-19T11:25:32Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
When CI has completed, please ensure no errors have appeared.

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

blathers-crl · 2020-05-19T11:40:14Z

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.}

abhishek20123g · 2020-05-19T11:51:42Z

TF[YT]R,
There seems to a lot of style-related review marks I need to be more careful ☺️.
Updated PR as you suggested.

otan · 2020-05-19T14:40:33Z

pkg/geo/geogfn/segmentize.go

+	spheroid := geographiclib.WGS84Spheroid
+	// Convert segmentMaxLength to s1.ChordAngle as further calculation
+	// is done considering segmentMaxLength as s1.ChordAngle.
+	segmentMaxLength = segmentMaxLength / spheroid.SphereRadius


make this of type s1.ChordAngle:
s1.ChordAngleFromAngle(s1.Angle(segmentMaxLength/spheroid.SphereRadius))

and rename it to segmentMaxChordAngle

This will be more efficient doing this way,
but I think s1.ChordAngle does not shows linear behaviour as

a := s1.ChordAngleFromAngle(s1.Angle(1)) b := s1.ChordAngleFromAngle(s1.Angle(0.5)) fmt.Println(a/b)

the expected ratio must be 2 but its actually 3.755165123780745
therefore, the ratio is not maintained after I convert them to s1.ChordAngle

otan · 2020-05-19T14:41:19Z

pkg/geo/geogfn/segmentize.go

+// the given maximum segment length.
+func segmentizeGeom(geometry geom.T, segmentMaxLength float64) (geom.T, error) {
+	if segmentMaxLength <= 0 {
+		return nil, errors.Newf("maximum segment length must be positive")


should this check be moved up to Segmentize?

Sure,
This should be in Segmentize in the first place

otan · 2020-05-19T14:43:23Z

pkg/geo/geogfn/segmentize.go

+		// (2^n)[numberOfSegmentToCreate] >= distanceBetweenPoints / segmentMaxLength > 2^(n-1)
+		// therefore n = ceil(log2(segmentMaxLength/distanceBetweenPoints)). Hence
+		// numberOfSegmentToCreate = 2^(ceil(log2(segmentMaxLength/distanceBetweenPoints))).
+		numberOfSegmentToCreate := int(math.Pow(2, math.Ceil(math.Log2(distanceBetweenPoints/maxSegmentLength))))


ok, this math is a little complicated. i think doing some inversing makes this cleaner:

numPoints := math.Ceil(maxSegmentLength / distanceBetweenPoints)

is there something wrong with doing it this way around?

I think there may be a problem with directly writing it that way,
for example,

postgis_db=# select st_distance('POINT(0 0)'::geography, 'POINT(1 1)'::geography, false); st_distance ----------------- 157249.59776851 (1 row) select st_astext(ST_Segmentize('LINESTRING(0 0, 1 1'::geography, 78624.798888)) postgis_db=# select st_astext(ST_Segmentize('LINESTRING(0 0, 1 1)'::geography, 78624.798888)); st_astext --------------------------------------------------------- LINESTRING(0 0,0.499961919922622 0.500019038226106,1 1) postgis_db=# select st_astext(ST_Segmentize('LINESTRING(0 0, 1 1)'::geography, 78623.798888)); st_astext --------------------------------------------------------------------------------------------------------------------------------- LINESTRING(0 0,0.249976200223564 0.250011898653433,0.499961919922622 0.500019038226106,0.749966679297834 0.750016659002919,1 1)

for the case, one ratio will be 1.99999
and the total number of segments was 2, and math.Ceil(distanceBetweenPoints/maxSegmentLength)
will as give 2.
and for case 2 ratio was 2.0002
and the total number of segments was 4, and math.Ceil(distanceBetweenPoints/maxSegmentLength)
will as give 3.

Looks like I misunderstood what this does. Thanks for setting me right.

otan · 2020-05-19T14:43:36Z

pkg/geo/geogfn/segmentize.go

+	point2 := s2.PointFromLatLng(s2.LatLngFromDegrees(b.Y(), b.X()))
+
+	var segmentizedPoints []s2.Point
+	distanceBetweenPoints := s2.ChordAngleBetweenPoints(point1, point2).Angle().Radians()


because of the above transformation, we don't need to do .Angle().Radians()

otan · 2020-05-19T14:46:19Z

pkg/geo/geogfn/segmentize.go

+// NOTE: List of points does not consist of end point.
+func segmentizeCoords(a geom.Coord, b geom.Coord, maxSegmentLength float64) []float64 {
+	// Converted geom.Coord into s2.Point so we can segmentize the coordinates.
+	point1 := s2.PointFromLatLng(s2.LatLngFromDegrees(a.Y(), a.X()))


nit: pointA, pointB.

otan · 2020-05-19T14:47:05Z

pkg/geo/geogfn/segmentize.go

+			segmentizedPoints = append(segmentizedPoints, newPoint)
+		}
+	}
+	allSegmentizedCoordinates := a.Clone()


seems like we're iterating over the array twice, and allocate two arrays twice. can we restructure this to look like:

segmentizedCoordinates := []float64{} segmentizedCoordinates = append(segmentizedCoordinates, a.Clone()) if maxSegmentLength <= distanceBetweenPoints { // .... for pointInserted := 1; pointInserted < numberOfSegmentToCreate; pointInserted++ { nextPoint := s2.Interpolate(....) segmentizedCoordinates = append(segmentizedCoordinates, nextPoint.Lng.Degrees(), latlng.Lat.Degrees()) } } return segmentizedCoordinates

otan · 2020-05-19T14:48:59Z

pkg/sql/sem/builtins/geo_builtins.go

+				return tree.NewDGeography(segGeometry), nil
+			},
+			Info: infoBuilder{
+				info: `turns a modified Geography having no segment longer than the given max_segment_length meters. 


nit: replace "turns" with "Returns". Extra new line after "meters." so that they appear in the autogenerated markdown as two different
tags.

otan · 2020-05-19T14:50:06Z

pkg/sql/sem/builtins/geo_builtins.go

+		tree.Overload{
+			Types: tree.ArgTypes{
+				{"geography", types.Geography},
+				{"max_segment_length", types.Float},


late realisation: maybe renaming this to max_segment_length_meters helps. now we can omit the "meters" word in the description!

(spelling metres as meters is painful for an aussie but here we are)

otan · 2020-05-20T02:23:33Z

pkg/geo/geo.go

@@ -188,6 +188,15 @@ func NewGeography(spatialObject geopb.SpatialObject) *Geography {
 	return &Geography{SpatialObject: spatialObject}
 }

+// NewGeographyFromGeom Geography Object from geom.T.


nit: "NewGeographyFromGeom creates a new Geography from a geom.T object."

abhishek20123g · 2020-05-20T13:11:59Z

Late to realise that for POINT and MULTIPOINT, St_Segmentize works with 0 and negative segment length.

postgis_db=# SELECT ST_AsText(ST_Segmentize('MULTIPOINT(0 0, 1 1)'::geography, -1));
      st_astext      
---------------------
 MULTIPOINT(0 0,1 1)

postgis_db=# SELECT ST_AsText(ST_Segmentize('LINESTRING(0 0, 1 1)'::geography, -1));
ERROR:  ptarray_segmentize_sphere: maximum segment length must be positive

updated PR according to this case and added their respective testcase in
both segmentize_test.go and geospatial file

otan · 2020-05-21T02:59:15Z

heads up this looks good to me - there's a few tests i want to add but they won't work yet because i have to land the patches for making them work (if you're curious, its strange types like LINESTRING EMPTY). i'll update your PR with this tomorrow and submit it. thanks for your contribution (and hope to see more :D).

otan · 2020-05-21T15:27:57Z

ok! this is ready to go. feeling like an extension? you can do this for geometry as well :P!
#49029

bors r+

otan · 2020-05-21T15:29:34Z

oops, lost your name in the commit, one sec

bors r-

craig · 2020-05-21T15:29:36Z

Canceled

Fixes: cockroachdb#48368 This PR adds ST_Segmentize({geography,float8}) builtin function which allows modify given geography such that no segment longer than the given max_segment_length. Co-authored-by: abhishek20123g <[email protected]> Release note (sql change): This PR implements the ST_Segmentize builtin function for Geography.

otan · 2020-05-21T15:44:27Z

bors r+

craig · 2020-05-21T16:59:04Z

Build failed (retrying...)

GitHub CI (Cockroach)

craig · 2020-05-21T18:38:44Z

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 18, 2020

blathers-crl bot requested a review from otan May 18, 2020 18:55

otan reviewed May 18, 2020

View reviewed changes

abhishek20123g force-pushed the geo-st_Segmentize branch from 9171e9e to ec20b10 Compare May 19, 2020 11:25

blathers-crl bot requested a review from otan May 19, 2020 11:25

abhishek20123g force-pushed the geo-st_Segmentize branch from ec20b10 to bf9c067 Compare May 19, 2020 11:40

abhishek20123g force-pushed the geo-st_Segmentize branch from bf9c067 to a36b0f9 Compare May 19, 2020 12:08

otan reviewed May 19, 2020

View reviewed changes

abhishek20123g force-pushed the geo-st_Segmentize branch from a36b0f9 to db6ab1c Compare May 19, 2020 18:31

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

otan reviewed May 20, 2020

View reviewed changes

abhishek20123g force-pushed the geo-st_Segmentize branch from db6ab1c to 9263099 Compare May 20, 2020 11:59

blathers-crl bot requested a review from otan May 20, 2020 11:59

abhishek20123g force-pushed the geo-st_Segmentize branch 2 times, most recently from 057e23b to 51bb1ed Compare May 20, 2020 13:04

otan force-pushed the geo-st_Segmentize branch from 51bb1ed to fa86da3 Compare May 21, 2020 15:26

otan force-pushed the geo-st_Segmentize branch from fa86da3 to 20eda8a Compare May 21, 2020 15:30

craig bot merged commit b1146e2 into cockroachdb:master May 21, 2020

builtin: Implements ST_Segmentize builtin function #49211

builtin: Implements ST_Segmentize builtin function #49211

Conversation

abhishek20123g commented May 18, 2020

blathers-crl bot commented May 18, 2020

cockroach-teamcity commented May 18, 2020

abhishek20123g commented May 18, 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

blathers-crl bot commented May 19, 2020

blathers-crl bot commented May 19, 2020

abhishek20123g commented May 19, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishek20123g commented May 20, 2020

otan commented May 21, 2020 • edited Loading

otan commented May 21, 2020

otan commented May 21, 2020

craig bot commented May 21, 2020

Canceled

otan commented May 21, 2020

craig bot commented May 21, 2020

Build failed (retrying...)

craig bot commented May 21, 2020

Build succeeded

otan commented May 21, 2020 •

edited

Loading