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.

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

[SEDONA-123] Fix check for invalid latitude/longitude values in ST_GeoHash #637

Merged
merged 2 commits into from
Jun 26, 2022
Merged

[SEDONA-123] Fix check for invalid latitude/longitude values in ST_GeoHash #637

merged 2 commits into from
Jun 26, 2022

Conversation

brianrice2
Copy link
Contributor

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

Currently, ST_GeoHash checks for invalid latitude and longitude values (in GeometryGeoHashEncoder.scala):

  • Longitude < -180
  • Latitude < -90
  • Longitude > 180
  • Longitude > 90

This PR fixes the last comparison to check latitude instead of longitude.

How was this patch tested?

Added unit tests that fail before the fix (as well as some passing tests for similar boundary cases), implemented the fix suggested by Itamar, and tests now pass again.

Some screenshots during the process:

Test results after adding tests but before fix:

Tests before overall

Tests before specific

Test results after fix:

Tests after

Did this PR include necessary documentation updates?

  • No, this PR does not affect any public API so no need to change the docs.

@jiayuasu jiayuasu merged commit c56f5d4 into apache:master Jun 26, 2022
@brianrice2 brianrice2 deleted the sedona-123 branch June 26, 2022 13:04
@brianrice2
Copy link
Contributor Author

Thanks @jiayuasu !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants