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

Update GeoPolygonFilter to handle polygons crossing the dateline #9339

Merged
merged 1 commit into from Jan 27, 2015

Conversation

Projects
None yet
5 participants
@nknize
Member

nknize commented Jan 16, 2015

PR #8672 addresses ambiguous polygons - those that either cross the dateline or span the map - by complying with the OGC standard right-hand rule. Since GeoPolygonFilter is self contained logic, the fix in #8672 did not address the issue for the GeoPolygonFilter. This was identified in issue #5968

This fixes the ambiguous polygon issue in GeoPolygonFilter by moving the dateline crossing code from ShapeBuilder to GeoUtils and reusing the logic inside the pointInPolygon method. Unit tests are added to ensure support for coordinates specified in either standard lat/lon or great-circle coordinate systems.

closes #5968
closes #9304

@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/GeoPoint.java
result = 31 * result + (int) (temp ^ (temp >>> 32));
return result;
}
public String toString() {
return "[" + lat + ", " + lon + "]";
return "[" + y + ", " + x + "]";

This comment has been minimized.

@rjernst

rjernst Jan 20, 2015

Member

I realize this was already this way, but the format here (using brackets) looks like GeoJSON, so shouldn't it be [x,y]?

@rjernst

rjernst Jan 20, 2015

Member

I realize this was already this way, but the format here (using brackets) looks like GeoJSON, so shouldn't it be [x,y]?

This comment has been minimized.

@nknize

nknize Jan 21, 2015

Member

To comply with GeoJSON, absolutely. I was a little concerned about changing order for sake of the existing Java API. If ES users are doing anything upstream with that string and we reverse order on them... But I do agree that this should comply with the GeoJSON spec, and so should they.

For consistency we should probably revisit the docs at http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/mapping-geo-point-type.html and the geo_point type in general. The order was originally set to comply with "natural language" lat/lon ordering to make life easy.

@nknize

nknize Jan 21, 2015

Member

To comply with GeoJSON, absolutely. I was a little concerned about changing order for sake of the existing Java API. If ES users are doing anything upstream with that string and we reverse order on them... But I do agree that this should comply with the GeoJSON spec, and so should they.

For consistency we should probably revisit the docs at http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/mapping-geo-point-type.html and the geo_point type in general. The order was originally set to comply with "natural language" lat/lon ordering to make life easy.

This comment has been minimized.

@nknize

nknize Jan 21, 2015

Member

Per our discussion (and the fact that reversing it ripples heavily through the GeoPointFilter and related tests) I'm going to leave this the way it is for now.

@nknize

nknize Jan 21, 2015

Member

Per our discussion (and the fact that reversing it ripples heavily through the GeoPointFilter and related tests) I'm going to leave this the way it is for now.

@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/GeoUtils.java
@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/GeoUtils.java
@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/GeoUtils.java
@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/GeoUtils.java
@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/GeoUtils.java
@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/GeoUtils.java
@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/GeoUtils.java
@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/GeoUtils.java
@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java
@rjernst

This comment has been minimized.

Show comment
Hide comment
@rjernst

rjernst Jan 21, 2015

Member

@nknize I left some comments.

Member

rjernst commented Jan 21, 2015

@nknize I left some comments.

@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Jan 23, 2015

Member

@rjernst please could you give another review

Member

clintongormley commented Jan 23, 2015

@rjernst please could you give another review

@rjernst

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/common/geo/GeoUtils.java
@rjernst

This comment has been minimized.

Show comment
Hide comment
@rjernst

rjernst Jan 27, 2015

Member

LGTM. I also think having dedicated unit tests for the utility methods in GeoUtils would be good, but what is here does not make anything worse (just moves existing code around).

Member

rjernst commented Jan 27, 2015

LGTM. I also think having dedicated unit tests for the utility methods in GeoUtils would be good, but what is here does not make anything worse (just moves existing code around).

[GEO] Update GeoPolygonFilter to handle ambiguous polygons
PR #8672 addresses ambiguous polygons - those that either cross the dateline or span the map - by complying with the OGC standard right-hand rule. Since ```GeoPolygonFilter``` is self contained logic, the fix in #8672 did not address the issue for the ```GeoPolygonFilter```. This was identified in issue #5968

This fixes the ambiguous polygon issue in ```GeoPolygonFilter``` by moving the dateline crossing code from ```ShapeBuilder``` to ```GeoUtils``` and reusing the logic inside the ```pointInPolygon``` method.  Unit tests are added to ensure support for coordinates specified in either standard lat/lon or great-circle coordinate systems.

closes #5968
closes #9304

@nknize nknize merged commit 06667c6 into elastic:master Jan 27, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details
@javanna

This comment has been minimized.

Show comment
Hide comment
@javanna

javanna Jan 28, 2015

Member

hey @nknize I am afraid this import might cause problems here. The jts dependency is optional in our pom, but this import here makes it mandatory I'm afraid?

hey @nknize I am afraid this import might cause problems here. The jts dependency is optional in our pom, but this import here makes it mandatory I'm afraid?

This comment has been minimized.

Show comment
Hide comment
@tlrx

tlrx Jan 28, 2015

Member

Looks like it's breaking many plugins http://build.elasticsearch.org/

Member

tlrx replied Jan 28, 2015

Looks like it's breaking many plugins http://build.elasticsearch.org/

@clintongormley clintongormley changed the title from [GEO] Update GeoPolygonFilter to handle polygons crossing the dateline to Geo: Update GeoPolygonFilter to handle polygons crossing the dateline Feb 10, 2015

@clintongormley clintongormley removed the review label Mar 19, 2015

@clintongormley clintongormley changed the title from Geo: Update GeoPolygonFilter to handle polygons crossing the dateline to Update GeoPolygonFilter to handle polygons crossing the dateline Jun 7, 2015

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