geo/geogfn: implement ST_Project by hueypark · Pull Request #49949 · cockroachdb/cockroach

hueypark · 2020-06-07T15:36:35Z

Fixes #48402

Release note (sql change): This PR implement adds the ST_Project({geography,float8,float8})

blathers-crl · 2020-06-07T15:36:40Z

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_Project({geography,float8,float8}) #48402)

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

cockroach-teamcity · 2020-06-07T15:36:41Z

This change is

otan

Thanks! I have a few suggestions on making this cleaner.

otan · 2020-06-07T19:50:56Z

pkg/geo/geogfn/unary_operators.go

+	}
+
+	// Normalize azimuth
+	azimuth -= 2.0 * math.Pi * math.Floor(azimuth/(2.0*math.Pi))


if we are using s1.Angle(), there is a Normalized function we can use.

otan · 2020-06-07T19:51:20Z

pkg/geo/geogfn/unary_operators.go

+
+	// Check the distance validity.
+	if distance > (math.Pi * spheroid.Radius) {
+		return nil, errors.Newf("Distance must not be greater than %v", math.Pi*spheroid.Radius)


nit: errors start with lowercase.
does this error also appear in PostGIS? can we use %f instead?'

Done.
Yes, PostGIS has the same error.
https://github.com/postgis/postgis/blob/65095c6c0eaa188cb2b36a6274d6a9c77cf7c5ea/liblwgeom/lwgeodetic.c#L2119

otan · 2020-06-07T19:52:16Z

pkg/geo/geogfn/unary_operators.go

+	y := point.Y()
+
+	projected := spheroid.Project(
+		s2.LatLng{Lat: s1.Angle(x), Lng: s1.Angle(y)},


i believe you can use s2.LatLngFromDegrees(point.Y(), point.X()). I also Lat is x, Lng is y.

otan · 2020-06-07T19:52:41Z

pkg/geo/geogfn/unary_operators.go

 }
+
+// longitudeNormalize convert a longitude to the range of -Pi, Pi.
+func longitudeNormalize(lon float64) float64 {


can we convert this to s1.Angle, and use s1.Angle(blah).Normalized()?

otan · 2020-06-07T19:53:43Z

pkg/geo/geographiclib/geographiclib.go

+
+	C.geod_direct(
+		&s.cRepr,
+		C.double(math.Pi*point.Lat/180.0),


we can use point.Lat.Degrees() for this, i believe.

otan · 2020-06-07T19:54:30Z

pkg/geo/geographiclib/geographiclib.go

+		nil)
+
+	return s2.LatLng{
+		Lat: s1.Angle(float64(lat) * math.Pi / 180.0),


s1.Angle(lat * s1.Degree) may be good here.

I fixed this with s2.LatLngFromDegrees.

otan · 2020-06-07T19:56:43Z

pkg/sql/sem/builtins/geo_builtins.go

+The distance is given in meters. Negative values are supported.
+
+The azimuth (also known as heading or bearing) is given in radians. It is measured clockwise from true north (azimuth zero).
+East is azimuth π/2 (90 degrees); south is azimuth π (180 degrees); west is azimuth 3π/2 (270 degrees).


if you wanted extra new lines here to display in HTML format, you'll need an extra new line here.
This new line is otherwise displayed as spaces (see autogenerated code and how the
tags are missing.)

I've seen functions.md, but I can't find anything wrong about the format. Can you explain it in more detail?

The azimuth (also known as heading or bearing) is given in radians. It is measured clockwise from true north (azimuth zero). East is azimuth π/2 (90 degrees); south is azimuth π (180 degrees); west is azimuth 3π/2 (270 degrees). Negative azimuth values and values greater than 2π (360 degrees) are supported.

if you wanted new line between them, you'd want

The azimuth (also known as heading or bearing) is given in radians. It is measured clockwise from true north (azimuth zero). East is azimuth π/2 (90 degrees); south is azimuth π (180 degrees); west is azimuth 3π/2 (270 degrees). Negative azimuth values and values greater than 2π (360 degrees) are supported.

then you need a new line between each here. if you're ok with just space difference, then this is ok. i'm up for either.

Thank you for explain. I want to keep it as it is.

hueypark · 2020-06-08T15:06:47Z

Thank you for the review.

blathers-crl · 2020-06-08T15:06:52Z

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

two small things -- otherwise LGTM! thank you so much!

otan · 2020-06-08T15:13:51Z

pkg/geo/geogfn/unary_operators.go

 }
+
+// latitudeNormalize convert a latitude to the range of -Pi/2, Pi/2.
+func latitudeNormalize(lat float64) float64 {


nit: can you name this normalizeLatitude?
also shame there isn't something similar in s1/s2 :(

otan · 2020-06-08T15:14:23Z

pkg/geo/geographiclib/geographiclib.go

+		C.double(distance),
+		&lat,
+		&lng,
+		nil)


nit: i prefer trailing commas and one arg per line, i.e.

nil, )

easier to git blame and less merge conflict-y.

otan · 2020-06-08T15:15:24Z

pkg/geo/geogfn/unary_operators.go

+	projected := spheroid.Project(
+		s2.LatLngFromDegrees(x, y),
+		distance,
+		azimuth)


trailing comma here too, and on geom.NewPointFlat

Fixes cockroachdb#48402 Release note (sql change): This PR implement adds the ST_Project({geography,float8,float8})

otan · 2020-06-08T21:58:50Z

bors r+

craig · 2020-06-08T22:35:24Z

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

blathers-crl bot requested a review from otan June 7, 2020 15:36

otan suggested changes Jun 7, 2020

View reviewed changes

hueypark force-pushed the st_project branch from 4cbb142 to da86d2d Compare June 8, 2020 15:06

blathers-crl bot requested a review from otan June 8, 2020 15:06

otan reviewed Jun 8, 2020

View reviewed changes

hueypark force-pushed the st_project branch from da86d2d to e2e5f83 Compare June 8, 2020 21:49

blathers-crl bot requested a review from otan June 8, 2020 21:49

hueypark force-pushed the st_project branch from e2e5f83 to 2ef9714 Compare June 8, 2020 21:52

geo/geogfn: implement ST_Project

5cbdb0d

Fixes cockroachdb#48402 Release note (sql change): This PR implement adds the ST_Project({geography,float8,float8})

hueypark force-pushed the st_project branch from 2ef9714 to 5cbdb0d Compare June 8, 2020 21:54

otan approved these changes Jun 8, 2020

View reviewed changes

craig bot merged commit cb34350 into cockroachdb:master Jun 8, 2020

Conversation

hueypark commented Jun 7, 2020

Uh oh!

blathers-crl bot commented Jun 7, 2020

Uh oh!

cockroach-teamcity commented Jun 7, 2020

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hueypark commented Jun 8, 2020

Uh oh!

blathers-crl bot commented Jun 8, 2020

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otan commented Jun 8, 2020

Uh oh!

craig bot commented Jun 8, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants