geo/geogfn: implement ST_Project #49949
Conversation
|
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: 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
|
This change is |
blathers-crl
bot
added
O-community
pkg/geo/geogfn/unary_operators.go
Outdated
| } | ||
|
|
||
| // Normalize azimuth | ||
| azimuth -= 2.0 * math.Pi * math.Floor(azimuth/(2.0*math.Pi)) |
There was a problem hiding this comment.
if we are using s1.Angle(), there is a Normalized function we can use.
pkg/geo/geogfn/unary_operators.go
Outdated
|
|
||
| // 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) |
There was a problem hiding this comment.
nit: errors start with lowercase.
does this error also appear in PostGIS? can we use %f instead?'
There was a problem hiding this comment.
Done.
Yes, PostGIS has the same error.
https://github.com/postgis/postgis/blob/65095c6c0eaa188cb2b36a6274d6a9c77cf7c5ea/liblwgeom/lwgeodetic.c#L2119
pkg/geo/geogfn/unary_operators.go
Outdated
| y := point.Y() | ||
|
|
||
| projected := spheroid.Project( | ||
| s2.LatLng{Lat: s1.Angle(x), Lng: s1.Angle(y)}, |
There was a problem hiding this comment.
i believe you can use s2.LatLngFromDegrees(point.Y(), point.X()). I also Lat is x, Lng is y.
pkg/geo/geogfn/unary_operators.go
Outdated
| @@ -128,3 +166,65 @@ func length(regions []s2.Region, useSphereOrSpheroid UseSphereOrSpheroid) (float | |||
| } | |||
| return totalLength, nil | |||
| } | |||
|
|
|||
| // longitudeNormalize convert a longitude to the range of -Pi, Pi. | |||
| func longitudeNormalize(lon float64) float64 { | |||
There was a problem hiding this comment.
can we convert this to s1.Angle, and use s1.Angle(blah).Normalized()?
|
|
||
| C.geod_direct( | ||
| &s.cRepr, | ||
| C.double(math.Pi*point.Lat/180.0), |
There was a problem hiding this comment.
we can use point.Lat.Degrees() for this, i believe.
| nil) | ||
|
|
||
| return s2.LatLng{ | ||
| Lat: s1.Angle(float64(lat) * math.Pi / 180.0), |
There was a problem hiding this comment.
s1.Angle(lat * s1.Degree) may be good here.
There was a problem hiding this comment.
I fixed this with s2.LatLngFromDegrees.
| 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). |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
I've seen functions.md, but I can't find anything wrong about the format. Can you explain it in more detail?
There was a problem hiding this comment.
<p>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.</p>
if you wanted new line between them, you'd want
<p>The azimuth (also known as heading or bearing) is given in radians. It is measured clockwise from true north (azimuth zero).</p>
<p>East is azimuth π/2 (90 degrees); south is azimuth π (180 degrees); west is azimuth 3π/2 (270 degrees).</p>
<p>Negative azimuth values and values greater than 2π (360 degrees) are supported.</p>
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.
There was a problem hiding this comment.
Thank you for explain. I want to keep it as it is.
|
Thank you for the review. |
|
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. |
pkg/geo/geogfn/unary_operators.go
Outdated
| @@ -128,3 +166,32 @@ func length(regions []s2.Region, useSphereOrSpheroid UseSphereOrSpheroid) (float | |||
| } | |||
| return totalLength, nil | |||
| } | |||
|
|
|||
| // latitudeNormalize convert a latitude to the range of -Pi/2, Pi/2. | |||
| func latitudeNormalize(lat float64) float64 { | |||
There was a problem hiding this comment.
nit: can you name this normalizeLatitude?
also shame there isn't something similar in s1/s2 :(
| C.double(distance), | ||
| &lat, | ||
| &lng, | ||
| nil) |
There was a problem hiding this comment.
nit: i prefer trailing commas and one arg per line, i.e.
nil,
)
easier to git blame and less merge conflict-y.
pkg/geo/geogfn/unary_operators.go
Outdated
| projected := spheroid.Project( | ||
| s2.LatLngFromDegrees(x, y), | ||
| distance, | ||
| azimuth) |
There was a problem hiding this comment.
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})
|
bors r+ |
Build succeeded |
Fixes #48402
Release note (sql change): This PR implement adds the ST_Project({geography,float8,float8})