geo/geogfn: implement ST_Azimuth({geometry,geometry})#50188
geo/geogfn: implement ST_Azimuth({geometry,geometry})#50188craig[bot] merged 1 commit intocockroachdb:masterfrom
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 |
f56dc8e to
f5c0cce
Compare
|
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/geomfn/azimuth_test.go
Outdated
| zero = 0.0 | ||
| aQuarterPi = 0.7853981633974483 | ||
| towQuartersPi = 1.5707963267948966 | ||
| threeQuartersPi = 2.356194490192344 |
There was a problem hiding this comment.
do you mind moving these vars into the TestAzimuth function? (unless we're planning to re-use them)
pkg/geo/geomfn/azimuth.go
Outdated
| return nil | ||
| } | ||
|
|
||
| azimuth := math.Mod(2*math.Pi+math.Pi/2-math.Atan2(b.Y()-a.Y(), b.X()-a.X()), 2*math.Pi) |
There was a problem hiding this comment.
do you mind adding a comment of why we're adding 2*math.Pi, i.e. we always want a positive number?
some commentary on that math would also be nice :)
pkg/geo/geomfn/azimuth.go
Outdated
| // Azimuth returns the azimuth in radians of the segment defined by the given point geometries. | ||
| // The azimuth is angle is referenced from north, and is positive clockwise. | ||
| // North = 0; East = π/2; South = π; West = 3π/2. | ||
| func Azimuth(a *geom.Point, b *geom.Point) *float64 { |
There was a problem hiding this comment.
actually, do you mind moving the logic in geo_builtins such that Azimuth only takes in (a *geo.Geometry, b *geo.Geometry), to be consistent with the rest of the function signatures in here?
f5c0cce to
e5429ec
Compare
|
Thank you for the review! |
Fixes cockroachdb#48887 Release note (sql change): This PR implement adds the `ST_Azimuth({geometry,geometry})` builtin.
otan
left a comment
There was a problem hiding this comment.
thanks for your contribution!
|
bors r+ |
Build failed (retrying...) |
Build failed (retrying...) |
Build succeeded |
Fixes #48887
Release note (sql change): This PR implement adds the ST_Azimuth({geometry,geometry})