[GEO] Add support to ShapeBuilders for building Lucene geometry #35707
Conversation
nknize added :Analytics/Geo 
| Pinging @elastic/es-search-aggs |
This commit adds support for building lucene geometry from the ShapeBuilders. This is needed for integrating LatLonShape as the primary indexing approach for geo_shape field types. All unit and integration tests are updated to add randomization for testing both jts/s4j shapes and lucene shapes.
nknize force-pushed the bkdBackedShapes-ShapeBuilders branch from 8ba0cfb to ab3f2fe Compare 
jpountz left a comment
jpountz left a comment There was a problem hiding this comment.
Let's fix tests to test both the JTS and Lucene shape on every test run rather than randomly testing one of them?
server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/search/geo/GeoFilterIT.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/common/geo/ShapeBuilderTests.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java Outdated Show resolved Hide resolved
| public abstract T build(); | ||
| public abstract T buildS4J(); | ||
| | ||
| public abstract Object buildLucene(); |
There was a problem hiding this comment.
If I understand it correctly, this is the future way of building shapes. So, would it make sense to call this method simply build()?
There was a problem hiding this comment.
Maybe in the future we want to add a new topology library, e.g. spatial3d, so I think it is better to name the method with a reference to the underlaying library.
There was a problem hiding this comment.
I think if we eventually end up supporting spatial3d or other packages then a more sophisticated factory pattern would be appropriate. But I chose this simple naming approach because its going to be removed / deleted in 7.0. So I didn't want to get too extravagant
server/src/main/java/org/elasticsearch/common/geo/builders/GeometryCollectionBuilder.java Outdated Show resolved Hide resolved
| } | ||
| | ||
| @Override | ||
| public Object buildLucene() { |
There was a problem hiding this comment.
I think having this return type as a parameter type would have helped me to understand the builder better.
There was a problem hiding this comment.
I think if these factory methods were hanging around longer we could/should go that way, but buildS4J is being removed in 7.0, so I was trying to keep it simple / straight-forward for removal.
I think we could go this route if/when we add something like spatial3d in the future.
server/src/main/java/org/elasticsearch/common/geo/ShapeRelation.java Outdated Show resolved Hide resolved
| Okay.. I think all comments have been addressed in the first round review. I removed the |
jpountz left a comment
jpountz left a comment There was a problem hiding this comment.
Left some minor comments, otherwise LGTM
| if (lon > 180d || lon <= -180d) { | ||
| return centeredModulus(lon, 360); | ||
| } | ||
| return lon + 0d; |
There was a problem hiding this comment.
can you add a comment that this aims at avoiding negative zeros? Also it's not obvious to me that centerModulus may never return negative zero should we do the same? Also we do we want to avoid negative zeros?
There was a problem hiding this comment.
I think it would be good to avoid negative 0. We can look at adding that to the parser in a separate PR? For now I'll add the comment and cleanup so that if centerModulus does return -0.0 its handled appropriately
server/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java Show resolved Hide resolved
server/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java Outdated Show resolved Hide resolved
| Made changes... will keep an eye on ci tests and merge when they go green |
| for (ShapeBuilder shape : this.shapes) { | ||
| Object o = shape.buildLucene(); | ||
| if (o.getClass().isArray()) { | ||
| shapes.addAll(Arrays.asList((Object[])o)); |
There was a problem hiding this comment.
Would this work for a collection of points?
There was a problem hiding this comment.
It does.. Arrays.asList on a double[][] multipoint type correctly adds two double[] points to the list as expected
This commit adds support for building lucene geometry from the ShapeBuilders. This is needed for integrating LatLonShape as the primary indexing approach for geo_shape field types. All unit and integration tests are updated to add randomization for testing both jts/s4j shapes and lucene shapes.
7 participants
This PR adds support for building lucene geometry from the ShapeBuilders. This is PR 1 of 2 for for integrating
LatLonShapeas the primary indexing approach forgeo_shapefield types. All unit and integration tests are updated to add randomization for testing both lucene shapes and jts/s4j shapes for bwc.relates #35320