Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

geo/geogfn: implement ST_Azimuth({geometry,geometry}) #50188

Merged
merged 1 commit into from
Jun 15, 2020
Merged

geo/geogfn: implement ST_Azimuth({geometry,geometry}) #50188

merged 1 commit into from
Jun 15, 2020
+176 −0

Conversation

hueypark
Copy link
Contributor

Fixes #48887

Release note (sql change): This PR implement adds the ST_Azimuth({geometry,geometry})

@blathers-crl
Copy link

blathers-crl bot commented Jun 14, 2020

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.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Jun 14, 2020
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@blathers-crl blathers-crl bot requested a review from otan June 14, 2020 09:38
@blathers-crl
Copy link

blathers-crl bot commented Jun 14, 2020

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
otan reviewed Jun 15, 2020
View reviewed changes
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick comment request :)

pkg/geo/geomfn/azimuth_test.go Outdated
zero = 0.0
aQuarterPi = 0.7853981633974483
towQuartersPi = 1.5707963267948966
threeQuartersPi = 2.356194490192344
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mind moving these vars into the TestAzimuth function? (unless we're planning to re-use them)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

otan
otan reviewed Jun 15, 2020
View reviewed changes
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 {
Copy link
Contributor

@otan otan Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@blathers-crl blathers-crl bot requested a review from otan June 15, 2020 16:43
@hueypark
Copy link
Contributor Author

Thank you for the review!

@hueypark @otan
geo/geogfn: implement ST_Azimuth({geometry,geometry})
24585a9 
Fixes cockroachdb#48887

Release note (sql change): This PR implement adds the
`ST_Azimuth({geometry,geometry})` builtin.
@otan otan force-pushed the st_azimuth_geom branch from e5429ec to 24585a9 Compare June 15, 2020 16:51
otan
otan approved these changes Jun 15, 2020
View reviewed changes
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your contribution!

@otan
Copy link
Contributor

otan commented Jun 15, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 15, 2020

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented Jun 15, 2020

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented Jun 15, 2020

Build succeeded

@craig craig bot merged commit 5cff000 into cockroachdb:master Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@otan otan otan approved these changes

Assignees
No one assigned
Labels
O-community Originated from the community X-blathers-triaged blathers was able to find an owner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

geo/geomfn: implement ST_Azimuth({geometry,geometry})
3 participants
@hueypark @cockroach-teamcity @otan