support ST_Contains with H3 optimization#7252
support ST_Contains with H3 optimization#7252vishwa35 wants to merge 5 commits intoapache:masterfrom
Conversation
| for (int i = 0; i < projectionBlock.getNumDocs(); i++) { | ||
| Geometry firstGeometry = GeometrySerializer.deserialize(firstValues[i]); | ||
| Geometry secondGeometry = GeometrySerializer.deserialize(secondValues[i]); | ||
| if (GeometryUtils.isGeography(firstGeometry) || GeometryUtils.isGeography(secondGeometry)) { |
There was a problem hiding this comment.
why did you remove this? performance?
There was a problem hiding this comment.
this check is needed, the current implementation does not handle geography correctly.
There was a problem hiding this comment.
why does it not work for geography?
| public class ScalarFunctions { | ||
|
|
||
| // from https://h3geo.org/docs/core-library/restable | ||
| private static final ImmutableMap<Double, Integer> RESOLUTIONS = ImmutableMap.<Double, Integer>builder() |
There was a problem hiding this comment.
plz move this to geo util class
There was a problem hiding this comment.
Also add more comments about the meaning of the value
| return H3Utils.H3_CORE.geoToH3(latitude, longitude, resolution); | ||
| } | ||
|
|
||
| public static List<Long> polygonToH3(List<Coordinate> region, int res) { |
| return Math.sqrt(max); | ||
| } | ||
|
|
||
| public static double dist(Coordinate a, Coordinate b) { |
There was a problem hiding this comment.
either make the unit an argument or add the unit as part of the func name
| for (int i = 0; i < projectionBlock.getNumDocs(); i++) { | ||
| Geometry firstGeometry = GeometrySerializer.deserialize(firstValues[i]); | ||
| Geometry secondGeometry = GeometrySerializer.deserialize(secondValues[i]); | ||
| if (GeometryUtils.isGeography(firstGeometry) || GeometryUtils.isGeography(secondGeometry)) { |
There was a problem hiding this comment.
this check is needed, the current implementation does not handle geography correctly.
| import org.roaringbitmap.buffer.MutableRoaringBitmap; | ||
|
|
||
| /** | ||
| * A filter operator that uses H3 index for geospatial data inclusion |
There was a problem hiding this comment.
plz add comments about the algorithm used in this operator
| // return filtered num_docs | ||
| MutableRoaringBitmap fullMatchDocIds = new MutableRoaringBitmap(); | ||
| for (long docId : _h3Ids) { | ||
| fullMatchDocIds.or(_h3IndexReader.getDocIds(docId)); |
There was a problem hiding this comment.
how do you determine full matches?
There was a problem hiding this comment.
please add unit tests to verify the algorithm
| * A filter operator that uses H3 index for geospatial data inclusion | ||
| */ | ||
| public class H3InclusionIndexFilterOperator extends BaseFilterOperator { | ||
| private static final String OPERATOR_NAME = "H3IndexFilterOperator"; |
There was a problem hiding this comment.
make operator name consistent with the class name?
5 participants
Description
Adds a second H3IndexFilterOperator specifically for inclusions; the former was designed for distance/radius calculations.
As a part of this change, a number of functions and logic to estimate a resolution were added to the ScalarFunctions. A hardcoded value currently exists in L71 of H3InclusionIndexFilterOperator that should probably we sorted out.
It also enables ST_Contains on geography coordinates, as there isn't anything different about that from geometry that should prohibit that? The geo coords are isomorphic wrt cartesian coords.
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notesand complete the section on Release Notes)Release Notes
Documentation