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

Fixes BoundingBox across complete longitudinal range #7340

Closed
wants to merge 1 commit into from

Conversation

@colings86
Copy link
Member

commented Aug 19, 2014

Adds a special case to the GeoBoundingBoxFilterParser so that the left of the box is not normalised int he case where left = -180 and right = 180. Before this change the left would be normalised to 180 in this case and the filter would only match points with a longitude of 180 (or -180).

Closes #5218

@colings86 colings86 added the review label Aug 19, 2014

@colings86 colings86 self-assigned this Aug 19, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2014

#5128 looks completely unrelated?

@jpountz

View changes

src/main/java/org/elasticsearch/index/query/GeoBoundingBoxFilterParser.java Outdated
GeoUtils.normalizePoint(topLeft);
// Special case: if the left is -180 and the right is 180 we are asking for the complete longitude range and don't want the left
// normalised to 180 otherwise it will only match points with a lon of 180
GeoUtils.normalizePoint(topLeft, true, !(left == -180 && right == 180));
GeoUtils.normalizePoint(bottomRight);

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 20, 2014

Contributor

Is there the same issue with latitudes?

This comment has been minimized.

Copy link
@colings86

colings86 Aug 20, 2014

Author Member

No, since latitudes don't wrap

@jpountz

View changes

src/main/java/org/elasticsearch/index/query/GeoBoundingBoxFilterParser.java Outdated
GeoUtils.normalizePoint(topLeft);
// Special case: if the left is -180 and the right is 180 we are asking for the complete longitude range and don't want the left
// normalised to 180 otherwise it will only match points with a lon of 180
GeoUtils.normalizePoint(topLeft, true, !(left == -180 && right == 180));

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 20, 2014

Contributor

Since this part is about normalization, I'm wondering how to handle the non-normalized case such as eg. left=0 and right=360?

This comment has been minimized.

Copy link
@colings86

colings86 Aug 27, 2014

Author Member

We could check for (right - left % 360 == 0 && right != left) and in this case just set left at -180 and right at 180? Feels a bit hacky though. what do you think @jpountz ?

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 28, 2014

Contributor

Agreed that it feels hacky, but this way it would work fine even when longitudes are not normalized so I think it is better? Maybe it should be (right - left % 360 == 0 && right > left) to make sure that the delta is > 0?

@jpountz jpountz removed the review label Aug 20, 2014

@clintongormley

This comment has been minimized.

Copy link
Member

commented Aug 22, 2014

@jpountz fixed the original issue to point to #5218

@colings86 colings86 added the review label Aug 27, 2014

@jpountz jpountz removed the review label Aug 28, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2014

@colings86 replied to your comment, your suggestion sounds good to me

@jpountz jpountz removed the review label Aug 28, 2014

@colings86 colings86 added the review label Sep 2, 2014

@colings86 colings86 force-pushed the colings86:fix/5128 branch Sep 2, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2014

LGTM

@jpountz jpountz removed the review label Sep 12, 2014

@colings86 colings86 force-pushed the colings86:fix/5128 branch Sep 12, 2014

Geo: Fixes BoundingBox across complete longitudinal range
Adds a special case to the GeoBoundingBoxFilterParser so that the left of the box is not normalised in the case where left and right are 360 apart.  Before this change the left would be normalised to 180 in this case and the filter would only match points with a longitude of 180 (or -180).

Closes #5128
@colings86

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2014

Pushed to master and 1.x

@colings86 colings86 closed this Sep 12, 2014

@colings86 colings86 deleted the colings86:fix/5128 branch Sep 15, 2014

@clintongormley clintongormley changed the title Geo: Fixes BoundingBox across complete longitudinal range Fixes BoundingBox across complete longitudinal range Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.