Add geohex_grid aggregation by VijayanB · Pull Request #160 · opensearch-project/geospatial

VijayanB · 2022-10-07T05:56:39Z

Description

This aggregation will use uber's h3 to group coordinates into H3 cell.
Created new aggregation type geohex_grid. The precision will be between 0 and 15. This aggreation has default precision as 5, similar to geohash and geotile.

Signed-off-by: Vijayan Balasubramanian balasvij@amazon.com

Check List

Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

navneet1v

I am not seeing IT tests for the new aggregation?

navneet1v

[Nit-pick] Overall comment on the code:

Can we consistent in use of var and classes while creating the local variables.
Please move all the public functions at the start of the class followed by protected and then private function. This improves reading and reviewing of the code.
Please be consistent in use of final key word. I would recommend to use final keyword in local variables and functions parameters. This aids that the variable reference will not be changed.

martin-gaievski · 2022-10-18T17:37:47Z

+import org.opensearch.search.aggregations.ParsedMultiBucketAggregation;
+import org.opensearch.test.InternalMultiBucketAggregationTestCase;
+
+// This class is modified from https://github.com/opensearch-project/OpenSearch/blob/main/modules/geo/src/test/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoGridTestCase.java


nit: I think we normally use block style comments for class:

/** * Class description */

martin-gaievski · 2022-10-18T17:39:12Z

+            JsonXContent.jsonXContent,
+            String.format(
+                Locale.getDefault(),
+                "{\"field\":\"my_loc\", \"precision\":\"%d\", \"size\": \"500\", \"shard_size\": \"550\"}",


looks like it's the same string we're using in multiple places, WDYT of making it a constant?

I refactored to a method in some places.

martin-gaievski · 2022-10-18T17:39:57Z

+import org.opensearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
+
+// This class is modified from https://github.com/opensearch-project/OpenSearch/blob/main/modules/geo/src/test/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java
+// to keep relevant test case required for GeoHex Grid Aggregation.


same as for other class, I think we use block comments for classes

martin-gaievski · 2022-10-18T17:43:01Z

+/**
+ * Helper class for H3 library
+ */
+public class GeoHexHelper {


just want to make sure that in this repo we're using Helper class suffix. I think in k-NN it's usually Util, but I finer with either one as long as we're consistent

Right. We are calling it a helper than a utility everywhere.

This aggregation will use uber's h3 to group coordinates into H3 cell. Created new aggregation type geohex_grid. The precision will be between 0 and 15. This aggreation has default precision as 5, similar to geohash and geotile. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

* Import h3 library (#154) Made following changes to make it compatible: 1. Rename package from elasticsearch to opensearch.geospatial 2. Update License headers 3. Update build file 4. Update settings to include sub projects * Use Transport Request (#164) Remove usage of deprecated BaseNodeRequest * Update http client package to resolve build failure (#168) (#171) * Introduce H3 min resolution constant (#165) H3 version 1 has 16 resolutions, numbered 0 through 15. Introduced a constant to represent min value, similar to max value. * Add geohex aggregation (#160) This aggregation will use uber's h3 to group coordinates into H3 cell. Created new aggregation type geohex_grid. The precision will be between 0 and 15. This aggreation has default precision as 5, similar to geohash and geotile. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> * Add integration test (#176) Included integration test for geohex_grid. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

…ch-project#179) * Import h3 library (opensearch-project#154) Made following changes to make it compatible: 1. Rename package from elasticsearch to opensearch.geospatial 2. Update License headers 3. Update build file 4. Update settings to include sub projects * Use Transport Request (opensearch-project#164) Remove usage of deprecated BaseNodeRequest * Update http client package to resolve build failure (opensearch-project#168) (opensearch-project#171) * Introduce H3 min resolution constant (opensearch-project#165) H3 version 1 has 16 resolutions, numbered 0 through 15. Introduced a constant to represent min value, similar to max value. * Add geohex aggregation (opensearch-project#160) This aggregation will use uber's h3 to group coordinates into H3 cell. Created new aggregation type geohex_grid. The precision will be between 0 and 15. This aggreation has default precision as 5, similar to geohash and geotile. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> * Add integration test (opensearch-project#176) Included integration test for geohex_grid. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

) * Import h3 library (#154) Made following changes to make it compatible: 1. Rename package from elasticsearch to opensearch.geospatial 2. Update License headers 3. Update build file 4. Update settings to include sub projects * Use Transport Request (#164) Remove usage of deprecated BaseNodeRequest * Update http client package to resolve build failure (#168) (#171) * Introduce H3 min resolution constant (#165) H3 version 1 has 16 resolutions, numbered 0 through 15. Introduced a constant to represent min value, similar to max value. * Add geohex aggregation (#160) This aggregation will use uber's h3 to group coordinates into H3 cell. Created new aggregation type geohex_grid. The precision will be between 0 and 15. This aggreation has default precision as 5, similar to geohash and geotile. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> * Add integration test (#176) Included integration test for geohex_grid. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

VijayanB requested a review from a team October 7, 2022 05:56

VijayanB self-assigned this Oct 7, 2022

VijayanB marked this pull request as draft October 7, 2022 05:57

VijayanB force-pushed the add-aggregation branch 5 times, most recently from af06606 to b0b350a Compare October 11, 2022 22:56

VijayanB marked this pull request as ready for review October 11, 2022 22:59

VijayanB force-pushed the add-aggregation branch from b0b350a to 3ab65e2 Compare October 11, 2022 23:09

navneet1v reviewed Oct 11, 2022

View reviewed changes

Comment thread ...g/opensearch/geospatial/search/aggregations/bucket/geogrid/GeoHexGridAggregationBuilder.java Outdated

navneet1v reviewed Oct 11, 2022

View reviewed changes

Comment thread src/main/java/org/opensearch/geospatial/stats/upload/UploadStatsNodeRequest.java

navneet1v reviewed Oct 11, 2022

View reviewed changes

Comment thread src/test/java/org/opensearch/geospatial/search/aggregations/bucket/geogrid/GeoHexGridTests.java Outdated

navneet1v reviewed Oct 11, 2022

View reviewed changes

Comment thread src/main/java/org/opensearch/geospatial/search/aggregations/bucket/geogrid/GeoHexHelper.java Outdated

Comment thread ...main/java/org/opensearch/geospatial/search/aggregations/bucket/geogrid/ParsedGeoHexGrid.java Outdated

VijayanB force-pushed the add-aggregation branch 3 times, most recently from a94c396 to c4daf9e Compare October 12, 2022 05:51

heemin32 reviewed Oct 12, 2022

View reviewed changes

VijayanB force-pushed the add-aggregation branch 3 times, most recently from 00547d7 to 7228275 Compare October 17, 2022 22:40

heemin32 approved these changes Oct 17, 2022

View reviewed changes

martin-gaievski reviewed Oct 18, 2022

View reviewed changes

VijayanB force-pushed the add-aggregation branch from 7228275 to ce75d61 Compare October 18, 2022 19:02

VijayanB requested a review from martin-gaievski October 18, 2022 19:34

martin-gaievski approved these changes Oct 18, 2022

View reviewed changes

VijayanB merged commit 22285e3 into opensearch-project:feature/h3-grid Oct 18, 2022

VijayanB mentioned this pull request Oct 24, 2022

Support Uber's H3 geospatial indexing system as geohex_grid #179

Merged

5 tasks

Conversation

VijayanB commented Oct 7, 2022

Description

Check List

Uh oh!

Uh oh!

Uh oh!

navneet1v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

navneet1v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants