-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add multipolygon support to GeoPolygonQueryBuilder #22237
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
Conversation
mikemccand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nknize, I left a bunch of small comments.
It would be nice if ES would accept a standard GeoJson multipolygon: Lucene has a simple parser for this, taking String and returning Polygon[] ... I think that'd be easier for people to use than a triple-nested array of lon/lat points ... but we should do that separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm do we also do lon then lat elsewhere in the ES APIs? This is very confusing ... Lucene standardized on lat then lon a while back!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In strings, the first value is the latitude ("lat,lon"), but in arrays, the first value is the longitude as per geojson (http://geojson.org/) spec ([lon, lat]). @nknize please correct me if I am wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include the actual points in the exc message to aid users debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, lat and lon are not used in this outer scope, only down inside the nested for loop? Can you move their declaration (the double[]) down there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for shellLat and shellLon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe assert poly.size() > 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some context to this exception so the user knows what part of their request was wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like these tests are a single polygon? Can you also test that holes are parsed & executed correctly, and more than one polygon too?
core/src/main/java/org/elasticsearch/index/query/GeoPolygonQueryBuilder.java
Outdated
Show resolved
Hide resolved
|
@nknize is this PR still a WIP? Is there someone else that should review it? |
|
I am sorry to say that I cannot use this way, it shows Elastic search version - 6.2.4 |
|
@polyfractal @nknize has this PR been supplanted by a different PR that provides the same functionality? if so, can you close please? |
LatLonPoint polygon query in Lucene 6 supports multipolygons and polygons with holes. This commit exposes this capability within elasticsearch.
8ec8e65 to
0cd36c5
Compare
|
Quick update on this. Nick rebased the PR onto master, looks like there are some CI failures related to backwards compatibility. In general the PR is almost done but probably needs some more testing and better/more informative exception messages. Nick doesn't have time to push this PR over the finish line right now, but we think it's still important so I'm going to mark this as a |
|
This is a good first issue for anyone interested in picking up some ES geo knowledge. The only thing missing is to add support for GeoJSON using lucene's |
|
Hello! What is the status of this PR? |
|
If no one is working on this in 2020, I'm gonna pick it up and try to get it finalized |
|
Thank you for the offer @nathancday but there is a 99% likelihood that we will be deprecating and removing |
|
I am going to close this as it does not apply any longer. |
LatLonPoint polygon query in Lucene 6 supports multipolygons and polygons with holes. This PR exposes this capability through the ES query DSL by adding
multipolygonsupport to theGeoPolygonQueryBuilderparser such that the following can now be parsed:To maintain backcompat the following is still supported:
The
GeoPolygonQueryBuildernow creates aPolygonobject and passes it toLatLonPoint.newPolygonQuery.NOTE:
TODO:
closes #20789