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

Geo: add validator that only checks altitude #43893

Merged
merged 4 commits into from
Jul 10, 2019

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Jul 2, 2019

By default, we don't check ranges while indexing geo_shapes. As a
result, it is possible to index geoshapes that contain contain
coordinates outside of -90 +90 and -180 +180 ranges. Such geoshapes
will currently break SQL and ML retrieval mechanism. This commit removes
these restriction from the validator is used in SQL and ML retrieval.

By default, we don't check ranges while indexing geo_shapes. As a
result, it is possible to index geoshapes that contain contains
coordinates outside of -90 +90 and -180 +180 ranges. Such geoshapes
will break SQL and ML retrieval mechanism. This commit removes these
restriction from the validator used in SQL and ML retrieval.
@imotov imotov added >bug :Analytics/Geo Indexing, search aggregations of geo points and shapes v8.0.0 v7.3.0 labels Jul 2, 2019
@imotov imotov requested review from talevy and nknize July 2, 2019 19:23
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

small nit in the javadoc, mostly LGTM

I have one question, which might be unrelated to this PR, but more of a general
testing consideration for GeometryVisitors. It feels like we should be testing
all calls to visit for each of the different shapes. is this being done somewhere
that I am not aware of?

@jpountz jpountz added v7.4.0 and removed v7.3.0 labels Jul 3, 2019
Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

thanks!

@imotov
Copy link
Contributor Author

imotov commented Jul 8, 2019

@elasticmachine test this please

@imotov imotov merged commit 6bd1853 into elastic:master Jul 10, 2019
imotov added a commit that referenced this pull request Jul 10, 2019
By default, we don't check ranges while indexing geo_shapes. As a
result, it is possible to index geoshapes that contain contain
coordinates outside of -90 +90 and -180 +180 ranges. Such geoshapes
will currently break SQL and ML retrieval mechanism. This commit removes
these restriction from the validator is used in SQL and ML retrieval.
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Nov 15, 2019
Relates: elastic/elasticsearch#43893

This commit removes the bound checks on latitude and longitude on GeoLocation (and the derived GeoCoordinate), to allow values outside of the -90 to 90, and -180 to 180 ranges, respecitvely.
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Nov 15, 2019
Relates: elastic/elasticsearch#43893

This commit removes the bound checks on latitude and longitude on GeoLocation (and the derived GeoCoordinate), to allow values outside of the -90 to 90, and -180 to 180 ranges, respecitvely.
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Nov 15, 2019
Relates: elastic/elasticsearch#43893

This commit removes the bound checks on latitude and longitude on GeoLocation (and the derived GeoCoordinate), to allow values outside of the -90 to 90, and -180 to 180 ranges, respecitvely.

(cherry picked from commit 29d68b2)
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Nov 15, 2019
Relates: elastic/elasticsearch#43893

This commit removes the bound checks on latitude and longitude on GeoLocation (and the derived GeoCoordinate), to allow values outside of the -90 to 90, and -180 to 180 ranges, respecitvely.

(cherry picked from commit 29d68b2)
@imotov imotov deleted the geo-disable-geography-validator branch May 1, 2020 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >bug v7.4.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants