Geo: replace intermediate geo objects with libs/geo #37721
Conversation
Replaces intermediate geo objects built by ShapeBuilders with objects from the libs/geo hierarchy. This should allow us to build all geo functionality around a single hierarchy. Follow up for elastic#35320
imotov
added
:Analytics/Geo
|
Pinging @elastic/es-analytics-geo |
|
@atorok I had to make a few changes, otherwise server couldn't find the dependencies for some reason. Could you take a look and see if there is a better way to do that? Thanks! |
libs/geo/build.gradle
Outdated
| @@ -21,6 +21,16 @@ apply plugin: 'elasticsearch.build' | |||
| apply plugin: 'nebula.maven-base-publish' | |||
| apply plugin: 'nebula.maven-scm' | |||
|
|
|||
| archivesBaseName = 'elasticsearch-geo' | |||
There was a problem hiding this comment.
This change should not be needed since we have project(":libs:geo").name = 'elasticsearch-geo' in settings.gradle. Do you get an error without this ?
There was a problem hiding this comment.
@atorok when I tried to do that I have got:
* Where:
Build file '/home/igor/Projects/Elastic/7.x/elasticsearch/build.gradle' line: 279
* What went wrong:
A problem occurred configuring root project 'elasticsearch'.
> Project :libs:geo not found.
There was a problem hiding this comment.
you have to reference it by the changed name as :libs:elasticsearch-geo
There was a problem hiding this comment.
I get the same thing only with a different name:
FAILURE: Build failed with an exception.
* Where:
Settings file '/home/igor/Projects/Elastic/7.x/elasticsearch/settings.gradle' line: 155
* What went wrong:
A problem occurred evaluating settings 'elasticsearch'.
> Project with path ':libs:elasticsearch-geo' could not be found.
There was a problem hiding this comment.
Maybe you forgot to revert the change in settings.gradle ?
This work for me (I can ./gradlew assemble):
diff --git a/build.gradle b/build.gradle
index 8f359365794..c611cb7cedb 100644
--- a/build.gradle
+++ b/build.gradle
@@ -212,7 +212,7 @@ allprojects {
"org.elasticsearch:elasticsearch-core:${version}": ':libs:core',
"org.elasticsearch:elasticsearch-nio:${version}": ':libs:nio',
"org.elasticsearch:elasticsearch-x-content:${version}": ':libs:x-content',
- "org.elasticsearch:elasticsearch-geo:${version}": ':libs:geo',
+ "org.elasticsearch:elasticsearch-geo:${version}": ':libs:elasticsearch-geo',
"org.elasticsearch:elasticsearch-secure-sm:${version}": ':libs:secure-sm',
"org.elasticsearch.client:elasticsearch-rest-client:${version}": ':client:rest',
"org.elasticsearch.client:elasticsearch-rest-client-sniffer:${version}": ':client:sniffer',
diff --git a/libs/geo/build.gradle b/libs/geo/build.gradle
index fdfdc5c7b12..ab3419b93b9 100644
--- a/libs/geo/build.gradle
+++ b/libs/geo/build.gradle
@@ -21,16 +21,6 @@ apply plugin: 'elasticsearch.build'
apply plugin: 'nebula.maven-base-publish'
apply plugin: 'nebula.maven-scm'
-archivesBaseName = 'elasticsearch-geo'
-
-publishing {
- publications {
- nebula {
- artifactId = archivesBaseName
- }
- }
-}
-
dependencies {
if (isEclipse == false || project.path == ":libs:geo-tests") {
testCompile("org.elasticsearch.test:framework:${version}") {
diff --git a/settings.gradle b/settings.gradle
index a9f08e53cb0..c098dc2c723 100644
--- a/settings.gradle
+++ b/settings.gradle
@@ -152,3 +152,4 @@ if (extraProjects.exists()) {
project(":libs:cli").name = 'elasticsearch-cli'
project(":libs:ssl-config").name = 'elasticsearch-ssl-config'
+project(":libs:geo").name = 'elasticsearch-geo'
There was a problem hiding this comment.
@atorok I could swear that I did exactly that and server couldn't pick up geo as a dependency :-) except... I just repeated that and it works. So I must have missed something. Thanks!
| @@ -67,6 +67,14 @@ public double getLon(int i) { | |||
| return lons[i]; | |||
| } | |||
|
|
|||
| public double[] getLats() { | |||
| return lats; | |||
There was a problem hiding this comment.
For safety can we change this to return lats.clone()?
| } | ||
|
|
||
| public double[] getLons() { | ||
| return lons; |
There was a problem hiding this comment.
Same here: return lons.clone()
| indexShape(context, o); | ||
| } | ||
| if (luceneShape instanceof Geometry) { | ||
| ((Geometry) luceneShape).visit(new GeometryVisitor<Void>() { |
There was a problem hiding this comment.
This is kind of hairy. Can we maybe have a protected member class that implements GeometryVisitor<Void> instead of doing this inline?
There was a problem hiding this comment.
I'm thinking it might be cleaner to do something like this
private void indexShape(ParseContext context, Object luceneShape) {
if (luceneShape instanceof Geometry) {
((Geometry) luceneShape).visit(new LuceneGeometryIndexer(context));
} else {
throw new IllegalArgumentException("invalid shape type found [" + luceneShape.getClass() + "] while indexing shape");
}
}
...
protected class LuceneGeometryIndexer implements GeometryVisitor<Void> {
...
There was a problem hiding this comment.
Good idea. I don't think it needs to be protected though. We can make it private here.
server/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java
Show resolved
Hide resolved
* elastic/master: (68 commits) Fix potential IllegalCapacityException in LLRC when selecting nodes (elastic#37821) Mute CcrRepositoryIT#testFollowerMappingIsUpdated Fix S3 Repository ITs When Docker is not Available (elastic#37878) Pass distribution type through to docs tests (elastic#37885) Mute SharedClusterSnapshotRestoreIT#testSnapshotCanceledOnRemovedShard SQL: Fix casting from date to numeric type to use millis (elastic#37869) Migrate o.e.i.r.RecoveryState to Writeable (elastic#37380) ML: removing unnecessary upgrade code (elastic#37879) Relax cluster metadata version check (elastic#37834) Mute TransformIntegrationTests#testSearchTransform Refactored GeoHashGrid unit tests (elastic#37832) Fixes for a few randomized agg tests that fail hasValue() checks Geo: replace intermediate geo objects with libs/geo (elastic#37721) Remove NOREPLACE for /etc/elasticsearch in rpm and deb (elastic#37839) Remove "reinstall" packaging tests (elastic#37851) Add unit tests for ShardStateAction's ShardStartedClusterStateTaskExecutor (elastic#37756) Exit batch files explictly using ERRORLEVEL (elastic#29583) TransportUnfollowAction should increase settings version (elastic#37859) AsyncTwoPhaseIndexerTests race condition fixed (elastic#37830) Do not allow negative variances (elastic#37384) ...
Replaces intermediate geo objects built by ShapeBuilders with
objects from the libs/geo hierarchy. This should allow us to build
all geo functionality around a single hierarchy.
Follow up for #35320