-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Adds support for geo-bounds filtering in geogrid aggregations #50002
Conversation
Pinging @elastic/es-analytics-geo (:Analytics/Geo) |
server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java
Show resolved
Hide resolved
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.
Overall it looks good. Left a few minor thoughts and suggestions. At minimum I think we should use the Lucene test utilities where we can. Separate from that I think we can introduce a little more randomization to the tests.
docs/reference/aggregations/bucket/geohashgrid-aggregation.asciidoc
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java
Outdated
Show resolved
Hide resolved
...a/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java
Outdated
Show resolved
Hide resolved
...est/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java
Show resolved
Hide resolved
Both geo_bounding_box query and geo_bounds aggregation have a very similar definition of a "bounding box". A lot of this logic (serialization, xcontent-parsing, etc) can be centralized instead of having separated efforts to do the same things
f9ae11c
to
0a69a33
Compare
It is fairly common to filter the geo point candidates in geohash_grid and geotile_grid aggregations according to some viewable bounding box. This change introduces the option of specifying this filter directly in the tiling aggregation.
0a69a33
to
af29e5e
Compare
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.
I think this is almost there, just want to think if we should use different implementations for the bounded and unbounded case when iterating doc values.
server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java
Outdated
Show resolved
Hide resolved
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.
lgtm
@elasticmachine update branch thanks Ignacio! |
@elasticmachine update branch |
…50996) * Adds support for geo-bounds filtering in geogrid aggregations (#50002) It is fairly common to filter the geo point candidates in geohash_grid and geotile_grid aggregations according to some viewable bounding box. This change introduces the option of specifying this filter directly in the tiling aggregation. This is even more relevant to `geo_shape` where the bounds will restrict the shape to be within the bounds this optional `bounds` parameter is parsed in an equivalent fashion to the bounds specified in the geo_bounding_box query.
…c#50002) It is fairly common to filter the geo point candidates in geohash_grid and geotile_grid aggregations according to some viewable bounding box. This change introduces the option of specifying this filter directly in the tiling aggregation. This is even more relevant to `geo_shape` where the bounds will restrict the shape to be within the bounds this optional `bounds` parameter is parsed in an equivalent fashion to the bounds specified in the geo_bounding_box query.
…fter elastic#50996 (elastic#50997) after elastic#50996 (backport of elastic#50002) merged, the version guard on geo-grid `bounds` parameter can be updated to 7.6 re-enables bwc tests
Relates: #4341, elastic/elasticsearch#50002 This commit adds support for supplying bounds on a geohash_grid aggregation, similar to how bounds are supplied on geo_bounding_box query.
Relates: #4341, elastic/elasticsearch#50002 This commit adds support for supplying bounds on a geohash_grid aggregation, similar to how bounds are supplied on geo_bounding_box query.
Relates: #4341, elastic/elasticsearch#50002 This commit adds support for supplying bounds on a geohash_grid aggregation, similar to how bounds are supplied on geo_bounding_box query.
) Relates: #4341, elastic/elasticsearch#50002 This commit adds support for supplying bounds on a geohash_grid aggregation, similar to how bounds are supplied on geo_bounding_box query. Co-authored-by: Russ Cam <russ.cam@elastic.co>
Relates: #4341, elastic/elasticsearch#50002 This commit adds support for supplying bounds on a geohash_grid aggregation, similar to how bounds are supplied on geo_bounding_box query. (cherry picked from commit 8ed67e3)
It is fairly common to filter the geo point candidates in
geohash_grid and geotile_grid aggregations according to some
viewable bounding box. This change introduces the option of
specifying this filter directly in the tiling aggregation.
This is even more relevant to
geo_shape
where the bounds will restrictthe shape to be within the bounds
this optional
bounds
parameter is parsed in an equivalent fashion tothe bounds specified in the geo_bounding_box query.
example: