geo: coerce invalid geography coordinates into geometry #50457
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. |
blathers-crl
bot
added
O-community
|
This change is |
|
@otan if you want this PR to mimic the behaviour exactly, i could add |
|
Thanks for doing this! This only applies to geography types - this code does geometry too. I also think the s2 library has normalise functions you can reuse. |
pkg/geo/parse.go
Outdated
|
|
||
| // longitudeNormalize will normalize angles to | ||
| // spherical polar angles (-180 to 180) | ||
| func longitudeNormalize(log float64) float64 { |
There was a problem hiding this comment.
any reason we can't use s2.Angle(angle).Normalized()?
There was a problem hiding this comment.
we should call this lng, log is something else
|
If you're still doing this see
#50708 and re-use those angle
normalization functions! The s1 ones clip incorrectly.
…On Mon, Jun 22, 2020 at 7:06 PM Oliver Tan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/geo/parse.go
<#50457 (comment)>
:
> + if lat > 90 {
+ lat = 180 - lat
+ }
+ if lat < -90 {
+ lat = -180 - lat
+ }
+
+ return lat
+}
+
+func makeValidPoint(lat float64, log float64) (float64, float64) {
+ if math.IsNaN(lat) || math.IsNaN(log) {
+ return lat, log
+ }
+ _lat := latitudeNormalize(lat)
+ _log := longitudeNormalize(log)
we usually do not use _ in Golang -- this should probably be a different
name.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#50457 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA32FQ4DFKASR7GKJPYAZK3RYAE3XANCNFSM4OEBI6LA>
.
--
Oliver Tan | Member of Technical Staff
Cockroach Labs
|
|
@otan yup! I was caught up other things last week, will update the pr today. Thanks for the insights. |
|
@otan I was able to get it running with your normalizing functions and s2.LatLngFromDegrees. Thanks. |
|
#50708 is being merged, but as a heads up, #50765 will affect you this too. #50765 means that you'll need to update the coordinates before we calculate the bounding box in if you don't want to wait, feel to rebase to #50708 and i'll take care of the merge with #50765 - otherwise if you could wait a day or so I'd be very appreciative. Again, thank you for dealing with this! |
|
All ready to go! Thank you for your patience. |
sooryranga
force-pushed
the
arun/fix_invalid_geography_coordinates
branch
from
258a664 to
e66767b
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. |
|
@otan updated the PR. Good call on using flatbuffers, makes it a lot more simpler. Thanks for the feedback! |
sooryranga
force-pushed
the
arun/fix_invalid_geography_coordinates
branch
from
e66767b to
a22c16f
Compare
This commit resolves: cockroachdb#50219 Previously geometry did not convert out of range lat-lngs into correct range. This commit mimics the behavior found in PostGIS. It converts out of range lat-lngs into correct range. Release note (sql change): Geometry now coerce invalid geography coordinates into correct geometry.
sooryranga
force-pushed
the
arun/fix_invalid_geography_coordinates
branch
from
a22c16f to
1184c63
Compare
|
bors r+ |
Build succeeded |
|
@Arun4rangan LAST DAY TO ORDER!!! |
This commit resolves: #50219
Previously geometry did not convert out of range lat-lngs into
correct range. This commit mimics the behavior found in PostGIS.
It converts out of range lat-lngs into correct range.
Release note (sql change): Geometry now coerce invalid
geography coordinates into correct geometry.