Use geohash cell instead of just a corner in geo_bounding_box #30698
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Treats geohashes as grid cells instead of just points when the geohashes are used to specify the edges in the geo_bounding_box query. For example, if a geohash is used to specify the top_left corner, the top left corner of the geohash cell will be used as the corner of the bounding box. Closes elastic#25154
imotov
added
>enhancement
>breaking
:Analytics/Geo
|
Pinging @elastic/es-search-aggs |
…r-geo-bounding-box-parsing-v2
jpountz
reviewed
May 23, 2018
| @@ -387,6 +387,21 @@ public static GeoPoint parseGeoPoint(Object value, final boolean ignoreZValue) t | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Represents the point of the geohash cell that should be used as the value of geohas | |||
|
|
||
| public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue) | ||
| throws IOException, ElasticsearchParseException { | ||
| return parseGeoPoint(parser, point, ignoreZValue, EffectivePoint.BOTTOM_LEFT); |
There was a problem hiding this comment.
can you add a comment about why we use the bottom left point here?
| @@ -515,7 +515,7 @@ public static Rectangle parseBoundingBox(XContentParser parser) throws IOExcepti | |||
| } | |||
| } | |||
| if (envelope != null) { | |||
| if ((Double.isNaN(top) || Double.isNaN(bottom) || Double.isNaN(left) || Double.isNaN(right)) == false) { | |||
| if ((Double.isNaN(top) && Double.isNaN(bottom) && Double.isNaN(left) && Double.isNaN(right)) == false) { | |||
There was a problem hiding this comment.
This is unrelated to my change. It's just a small bug that I found and fixed while working on this portion of the code during the previous iteration and left it in this one. I can move it to a separate PR if you prefer. This check is making sure that the users didn't try to specify both WKT and one of the corners, but I think it was written incorrectly. For example, if a user specifies the entire box using WKT and only the top left corner like in this test https://github.com/elastic/elasticsearch/pull/30698/files#diff-cdab848071ff8179d7b1d3c09c39dbffR498 In the old implementation we will get:
false || true || true || false which is true and we will not trigger the warning. Maybe I should rewrite it as:
if (Double.isNaN(top) == false || Double.isNaN(bottom) == false || Double.isNaN(left) == false || Double.isNaN(right) == false) {
throw new ElasticsearchParseException("failed to parse bounding box. Conflicting definition found "
+ "using well-known text and explicit corners.");
}
Would it make easier to understand?
There was a problem hiding this comment.
It totally would. Thanks for explaining.
jpountz
approved these changes
May 24, 2018
…r-geo-bounding-box-parsing-v2
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
May 24, 2018
* master: Update the version checks around ip_range bucket keys, now that the change was backported. Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges Use geohash cell instead of just a corner in geo_bounding_box (elastic#30698) Limit user to single concurrent auth per realm (elastic#30794) [Tests] Move templated _rank_eval tests (elastic#30679) Security: fix dynamic mapping updates with aliases (elastic#30787) Ensure that ip_range aggregations always return bucket keys. (elastic#30701) Use remote client in TransportFieldCapsAction (elastic#30838) Move Watcher versioning setting to meta field (elastic#30832) [Docs] Explain incomplete dates in range queries (elastic#30689) Move persistent task registrations to core (elastic#30755) Decouple ClusterStateTaskListener & ClusterApplier (elastic#30809) Send client headers from TransportClient (elastic#30803) Packaging: Ensure upgrade_is_oss flag file is always deleted (elastic#30732) Force stable file modes for built packages (elastic#30823) [DOCS] Fixes typos in security settings Fix GeoShapeQueryBuilder serialization after backport
martijnvg
added a commit
that referenced
this pull request
May 25, 2018
* es/master: Move score script context from SearchScript to its own class (#30816) Fix bad version check writing Repository nodes (#30846) [docs] explainer for java packaging tests (#30825) Remove Throwable usage from transport modules (#30845) REST high-level client: add put ingest pipeline API (#30793) Update the version checks around ip_range bucket keys, now that the change was backported. Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges Use geohash cell instead of just a corner in geo_bounding_box (#30698) Limit user to single concurrent auth per realm (#30794) [Tests] Move templated _rank_eval tests (#30679) Security: fix dynamic mapping updates with aliases (#30787) Ensure that ip_range aggregations always return bucket keys. (#30701) Use remote client in TransportFieldCapsAction (#30838) Move Watcher versioning setting to meta field (#30832) [Docs] Explain incomplete dates in range queries (#30689) Move persistent task registrations to core (#30755) Decouple ClusterStateTaskListener & ClusterApplier (#30809) Send client headers from TransportClient (#30803) Packaging: Ensure upgrade_is_oss flag file is always deleted (#30732) Force stable file modes for built packages (#30823)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Treats geohashes as grid cells instead of just points when the
geohashes are used to specify the edges in the geo_bounding_box
query. For example, if a geohash is used to specify the top_left
corner, the top left corner of the geohash cell will be used as the
corner of the bounding box.
Closes #25154