LUCENE-8126: Spatial prefix tree based on S2 geometry #302
Conversation
| if (cellId == null) { //World cell | ||
| return tree.getSpatialContext().getWorldBounds(); | ||
| } | ||
| return tree.s2ShapeFactory.getS2CellShape(cellId); |
There was a problem hiding this comment.
I think you meant to cache to this.shape instead?
| /** | ||
| * Cache level of cell. | ||
| */ | ||
| private void setLevel() { |
There was a problem hiding this comment.
it'd be simpler of getLevel had this logic.... is the s2's cellId.level() not trivially fast and so you want to effectively cache it?
There was a problem hiding this comment.
The method cellId.level() is probably efficient but because it can be called several times I think we are saving some ticks on the clock by caching it.
| /*Tokens are used to serialize cells*/ | ||
| private static final byte[] TOKENS; | ||
| /*Map containing mapping between tokens and integer values*/ | ||
| private static final Map<Byte,Integer> PIXELS; |
There was a problem hiding this comment.
Since this map has a small set of fixed values that have numeric equivalents, perhaps we can do direct addressing into an array?
| public Cell readCell(BytesRef term, Cell scratch) { | ||
| S2PrefixTreeCell cell = (S2PrefixTreeCell) scratch; | ||
| if (cell == null) | ||
| cell = (S2PrefixTreeCell) getWorldCell(); |
There was a problem hiding this comment.
nitpick: our code style in Lucene/Solr is to always use braces
| if (cellId == null){ // this is the world cell | ||
| children = FACES; | ||
| } | ||
| else { |
There was a problem hiding this comment.
nitpick: our code style in Lucene/Solr is for an else statement to be on the same line as the previous closing brace
| return; | ||
| } | ||
| int length = getLevel() + 1; | ||
| byte[] b = new byte[length]; |
There was a problem hiding this comment.
the bref parameter may already have bytes that is of this length. If it is, reuse it thus avoiding object allocation. BytesRef exists to avoid object allocation and copying.
| @Override | ||
| public boolean equals(Object o) { | ||
| S2PrefixTreeCell cell = (S2PrefixTreeCell)o; | ||
| if (cellId == null) { |
|
|
||
| @Override | ||
| public double getDistanceForLevel(int level) { | ||
| return S2Projections.MAX_WIDTH.getValue(level -1) * DistanceUtils.RADIANS_TO_DEGREES; |
There was a problem hiding this comment.
nitpick: put space after that minus operator
| Point p = (Point) shape; | ||
| S2CellId id = S2CellId.fromLatLng(S2LatLng.fromDegrees(p.getY(), p.getX())).parent(detailLevel-1); | ||
| List<Cell> cells = new ArrayList<>(detailLevel); | ||
| for (int i=0; i < detailLevel -1; i++) { |
There was a problem hiding this comment.
nitpick: put a space after that minus operator
…revert previous change as well.
| @@ -50,8 +53,20 @@ | |||
| private RecursivePrefixTreeStrategy rptStrategy; | |||
|
|
|||
| private void setupGeohashGrid() { | |||
There was a problem hiding this comment.
This method is now named inappropriately
| this.grid = new QuadPrefixTree(ctx, 5);//A fairly shallow grid | ||
| this.rptStrategy = newRPT(); | ||
| } | ||
| else { |
lucene/spatial-extras/ivy.xml
Outdated
| @@ -29,6 +29,8 @@ | |||
| <artifact name="spatial4j" type="test" ext="jar" maven:classifier="tests" /> | |||
| </dependency> | |||
|
|
|||
| <dependency org="io.sgr" name="s2-geometry-library-java" rev="1.0.0" conf="test"/> | |||
There was a problem hiding this comment.
Instead you're supposed to reference the version via ivy-versions.properties. Also run "ant jar-checksums" to add the checksum file.
|
BTW when you're finally ready to merge into master & 7.3 please add just one commit with all these changes; don't rebase or merge all these commits onto the ASF git repo. This will keep the history clean/simple. |
No description provided.