-
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
Geohex aggregation on geo_shape field #91956
Conversation
Hi @iverase, I've created a changelog YAML for you. |
Pinging @elastic/es-analytics-geo (Team:Analytics) |
@elasticmachine update branch |
# Conflicts: # x-pack/plugin/spatial/src/internalClusterTest/java/org/elasticsearch/xpack/spatial/search/GeoGridAggAndQueryConsistencyIT.java
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.
Mostly I like this a lot. I have, however, many comments, questions and suggestions. I'm approving this as a very good first step, with the expectation that we do more work before making this feature available. In particular increasing test coverage and fixing any bugs found by that process.
...arch/xpack/spatial/search/aggregations/bucket/geogrid/GeoShapeGeoHexGridAggregatorTests.java
Show resolved
Hide resolved
...g/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTiler.java
Show resolved
Hide resolved
.../elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/AbstractGeoHexGridTiler.java
Show resolved
Hide resolved
.../elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/AbstractGeoHexGridTiler.java
Show resolved
Hide resolved
.../elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/AbstractGeoHexGridTiler.java
Show resolved
Hide resolved
...lugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/common/H3CartesianUtilTests.java
Show resolved
Hide resolved
...lugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/common/H3CartesianUtilTests.java
Show resolved
Hide resolved
...lugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/common/H3CartesianUtilTests.java
Show resolved
Hide resolved
...ava/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexTilerTests.java
Show resolved
Hide resolved
...a/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexVisitorTests.java
Outdated
Show resolved
Hide resolved
} else { | ||
// points are solved on the spherical geometry | ||
final H3LatLonGeometry geometry = new H3LatLonGeometry(id); | ||
if (fieldType instanceof GeoPointFieldMapper.GeoPointFieldType pointFieldType) { |
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.
Could just have used the shared interface GeoShapeQueryable
…on/apis/maps/get_grid_tile.js (#148346) Fixes #148122 elastic/elasticsearch#91956 added Geohex aggregation on geo_shape field and also changed how resolutions are mapped on vector tiles. 85264a33fffffff has resolution 5 and 84264a3ffffffff has resolution 4. The new resolution mappings are in https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/vector-tile/src/main/java/org/elasticsearch/xpack/vectortile/rest/GridAggregation.java#L122. Resolution mapping was changed to avoid hitting bucket limits which are easier to hit with large shapes and hex binning on geo_shape. Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
The feature to add support for geohex_grid aggregations over geo_shape fields was added in elastic#91956. This is the associated documentation for that.
* Documentation for geohex_grid over geo_shape The feature to add support for geohex_grid aggregations over geo_shape fields was added in #91956. This is the associated documentation for that. * Update docs/reference/aggregations/bucket/geohexgrid-aggregation.asciidoc Co-authored-by: Abdon Pijpelink <abdon.pijpelink@elastic.co> * Fix explanation for geo_point vs geo_shape proj When aggregating geohex over geoshape we use requirectangular because underlying lucene index indexes and searches the polygons in that way. * Correct spelling According to grammarly, "therefor" is not an alternative spelling of "therefore". We should use the conjunctive form here. See https://www.grammarly.com/blog/therefore-vs-therefor/ Co-authored-by: Abdon Pijpelink <abdon.pijpelink@elastic.co>
Implementation of geohex aggregation on geo_shape fields. It projects the H3 grid system into a equirectangular projection (see H3CartesianUtil) and uses it to rasterise the geometries indexed on a geo_shape field. One important side effect of this projection is that the relationship between the cell containing a pole and its children maybe lost so it requires special treatment.
Most the rasterization logic is inside
AbstractGeoHexGridTiler
. It is important to understand that children of an H3 bin are not fully contain in the parent and an H3 bin intersects other h3 bin in the next level which are not in the children set. Those bins can be found using H3 methodH3.h3ToNoChildrenIntersecting
. In order to make sure we visit all necessary cells we add the following call when navigating down the H3 hierarchy: