Add geohex_grid aggregation #160

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 [email protected]

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.

...g/opensearch/geospatial/search/aggregations/bucket/geogrid/GeoHexGridAggregationBuilder.java

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

navneet1v

I am not seeing IT tests for the new aggregation?

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

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.

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

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

libs/h3/src/main/java/org/opensearch/geospatial/h3/H3.java

libs/h3/src/test/java/org/opensearch/geospatial/h3/ParentChildNavigationTests.java

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

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

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

+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

...java/org/opensearch/geospatial/search/aggregations/bucket/geogrid/GeoHexGridParserTests.java

+            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

.../org/opensearch/geospatial/search/aggregations/bucket/geogrid/GeoHexGridAggregatorTests.java

+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

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

+/**
+ * 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 <[email protected]>

* 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 <[email protected]> * Add integration test (#176) Included integration test for geohex_grid. Signed-off-by: Vijayan Balasubramanian <[email protected]>

…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 <[email protected]> * Add integration test (opensearch-project#176) Included integration test for geohex_grid. Signed-off-by: Vijayan Balasubramanian <[email protected]>

) * 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 <[email protected]> * Add integration test (#176) Included integration test for geohex_grid. Signed-off-by: Vijayan Balasubramanian <[email protected]>

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

...g/opensearch/geospatial/search/aggregations/bucket/geogrid/GeoHexGridAggregationBuilder.java Outdated Show resolved Hide resolved

navneet1v reviewed Oct 11, 2022

View reviewed changes

src/main/java/org/opensearch/geospatial/stats/upload/UploadStatsNodeRequest.java Outdated Show resolved Hide resolved

navneet1v reviewed Oct 11, 2022

View reviewed changes

src/test/java/org/opensearch/geospatial/search/aggregations/bucket/geogrid/GeoHexGridTests.java Outdated Show resolved Hide resolved

navneet1v reviewed Oct 11, 2022

View reviewed changes

src/main/java/org/opensearch/geospatial/search/aggregations/bucket/geogrid/GeoHexHelper.java Outdated Show resolved Hide resolved

...main/java/org/opensearch/geospatial/search/aggregations/bucket/geogrid/ParsedGeoHexGrid.java Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add geohex_grid aggregation #160

Add geohex_grid aggregation #160

VijayanB commented Oct 7, 2022

navneet1v left a comment

navneet1v left a comment

martin-gaievski Oct 18, 2022

VijayanB Oct 18, 2022

martin-gaievski Oct 18, 2022

VijayanB Oct 18, 2022

martin-gaievski Oct 18, 2022

VijayanB Oct 18, 2022

martin-gaievski Oct 18, 2022

VijayanB Oct 18, 2022

Add geohex_grid aggregation #160

Add geohex_grid aggregation #160

Conversation

VijayanB commented Oct 7, 2022

Description

Check List

navneet1v left a comment

Choose a reason for hiding this comment

navneet1v left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment