Geo: Adds a set of no dependency geo classes for JDBC driver by imotov · Pull Request #36477 · elastic/elasticsearch

imotov · 2018-12-11T13:56:17Z

This set of geo classes is meant to be used in the JDBC driver to represent geo data and possibly as an intermediate format to pass geo shapes for indexing in #35320.

Relates to #35767 and #35320

WIP

Removes all math and WKB parser and refactors WKT parser

elasticmachine · 2018-12-11T13:56:19Z

Pinging @elastic/es-analytics-geo

alpar-t

I'm missing some context here, but left some questions around the build.

alpar-t · 2018-12-11T15:49:42Z

libs/geo/build.gradle

+    }
+}
+
+jarHell.enabled = false


Why don't we want to check for jar hell ?

alpar-t · 2018-12-11T15:55:26Z

libs/geo/build.gradle

+
+    testCompile "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
+    testCompile "junit:junit:${versions.junit}"
+    testCompile "org.hamcrest:hamcrest-all:${versions.hamcrest}"


Aren't these transitive dependencies of the test framework ?

elasticmachine · 2018-12-11T16:03:12Z

Pinging @elastic/es-core-infra

alpar-t · 2018-12-12T09:51:50Z

libs/geo/build.gradle

+    testCompile "junit:junit:${versions.junit}"
+    testCompile "org.hamcrest:hamcrest-all:${versions.hamcrest}"
+
+    if (isEclipse == false || project.path == ":libs:x-content-tests") {


This should be :libs:geo-tests which needs to be defined in settings.gradle to keep eclipse happy.

Note to self: We could simplify this setup for the latest Eclipse version (gradle/gradle#4802).

alpar-t · 2018-12-12T09:56:39Z

libs/geo/build.gradle

+}
+
+dependencies {
+    compile "org.elasticsearch:elasticsearch-core:${version}"


Is this needed ? Couldn't find any code that uses it.

alpar-t

Thanks @imotov this look much better.
Left some more comments, sorry I didn't notice the publishing in the first round.

alpar-t · 2018-12-12T14:38:52Z

libs/geo/build.gradle

+
+apply plugin: 'elasticsearch.build'
+apply plugin: 'nebula.maven-base-publish'
+apply plugin: 'nebula.maven-scm'


Are we planning to publish this jar e.x to maven central ?
Note that we'll have to update the release manager in that case.
We should only configure publishing if we do plan to publish it.

That's a good question. Its content will be shipped as a part of fat jdbc jar for SQL, but it will also used inside elasticsearch itself for geo operations. So that probably means that it needs to be published, correct?

That's right, don't forget to add it to release manager.

alpar-t · 2018-12-12T14:39:54Z

libs/geo/build.gradle

+}
+
+dependencies {
+    if (isEclipse == false || project.path == ":libs:x-geo-tests") {


the project should be :libs:geo-tests and it needs to be created in settings.gradle

alpar-t · 2018-12-13T16:01:52Z

settings.gradle

  projects << 'libs:x-content-tests'
  projects << 'libs:secure-sm-tests'
  projects << 'libs:grok-tests'
+  projects << 'libs:geo-tests'


There's another conditional for eclipse, line 104 that needs to be updated.

alpar-t

Thanks @imotov . One nit, the build LGMT otherwise.

alpar-t · 2018-12-17T07:10:14Z

libs/geo/build.gradle

+apply plugin: 'nebula.maven-base-publish'
+apply plugin: 'nebula.maven-scm'
+
+archivesBaseName = 'elasticsearch-geo'


NIT: you could have project(':libs:geo').name = "elasticsearch-geo" in settings.gradle instead of this. That would make the publishing block bellow unnecessary and it has the advantage of showing the module name in the IDE as well.

imotov · 2019-01-02T21:38:33Z

@elasticmachine run gradle build tests 2

libs/geo/src/main/java/org/elasticsearch/geo/GeoUtils.java

libs/geo/src/main/java/org/elasticsearch/geo/geometry/GeometryVisitor.java

iverase · 2019-01-08T11:25:06Z

Thanks, let's wait for Nick's review before merging

nknize

LGTM. I left a couple minor comments that I think could be added in a future PR so we don't hold this up any longer. The big thing would be to start moving our random geometry generators to lucene's GeoTestUtil. Lets open a separate issue for that.

nknize · 2019-01-07T15:45:50Z

libs/geo/src/main/java/org/elasticsearch/geo/GeoUtils.java

+/**
+ * Basic reusable geo-spatial utility methods
+ */
+public final class GeoUtils {


There's also o.e.common.geo.GeoUtils I think we'll want to figure out how to best reconcile the two so that there aren't duplicate GeoUtils in two different packages.

Agree. I renamed it to GeometryUtils when I made changed its visibility.

libs/geo/src/main/java/org/elasticsearch/geo/geometry/GeometryVisitor.java

nknize · 2019-01-07T15:59:29Z

libs/geo/src/main/java/org/elasticsearch/geo/geometry/GeometryVisitor.java

+package org.elasticsearch.geo.geometry;
+
+/**
+ * Support class for creating of geometry Visitors


Lets add some good javadocs here to clearly explain the design concept.

nknize · 2019-01-08T19:38:45Z

libs/geo/src/test/java/org/elasticsearch/geo/geometry/BaseGeometryTestCase.java

+        return new Point(randomLat(), randomLon());
+    }
+
+    public static LinearRing randomLinearRing() {


I think we should reuse lucene's GeoTestUtil for creating these random geometries. There was a lot of work put in to making sure the test suite creates adversarial geometries that have a tendency to expose weaknesses in the API. I don't think that should hold up the PR, though. So lets create a separate issue to migrate some of the random geometry generators over to lucene's geo test suite.

I opened #37278 as a follow-up.

matriv · 2019-01-10T09:11:19Z

libs/geo/src/test/java/org/elasticsearch/geo/geometry/LinearRingTests.java

+import org.elasticsearch.geo.utils.WellKnownText;
+import org.elasticsearch.test.ESTestCase;
+
+public class LinearRingTests extends ESTestCase {


Why is it not extending the BaseGeometryTestCase ?

Ah Ignore... I see it's not supported by WKT...

matriv · 2019-01-10T10:06:59Z

libs/geo/src/main/java/org/elasticsearch/geo/geometry/Rectangle.java

+        this.maxLat = maxLat;
+        empty = false;
+        if (maxLat < minLat) {
+            throw new IllegalArgumentException("max lat cannot be less than min lat");


shouldn't be a check for minLon maxLon as well?

Nope, it is perfectly valid to have minLon > maxLon, it just means that the Rectangle crosses the dateline.

matriv · 2019-01-10T10:09:24Z

libs/geo/src/main/java/org/elasticsearch/geo/geometry/GeometryVisitor.java

+package org.elasticsearch.geo.geometry;
+
+/**
+ * Support class for creating of geometry Visitors.


you could remove of.

matriv · 2019-01-10T10:11:25Z

libs/geo/src/main/java/org/elasticsearch/geo/geometry/LinearRing.java

+public class LinearRing extends Line {
+    public static final LinearRing EMPTY = new LinearRing();
+
+


Minor: you could remove this empty line.

matriv · 2019-01-10T10:11:33Z

libs/geo/src/main/java/org/elasticsearch/geo/geometry/LinearRing.java

+
+
+    private LinearRing() {
+


matriv · 2019-01-10T10:11:46Z

libs/geo/src/main/java/org/elasticsearch/geo/geometry/MultiLine.java

+    public static final MultiLine EMPTY = new MultiLine();
+
+    private MultiLine() {
+


matriv · 2019-01-10T10:12:07Z

libs/geo/src/main/java/org/elasticsearch/geo/geometry/MultiPolygon.java

+    public static final MultiPolygon EMPTY = new MultiPolygon();
+
+    private MultiPolygon() {
+


matriv · 2019-01-10T10:17:23Z

libs/geo/src/main/java/org/elasticsearch/geo/utils/WellKnownText.java

+            tokenizer.wordChars('-', '-');
+            tokenizer.wordChars('+', '+');
+            tokenizer.wordChars('.', '.');
+            tokenizer.whitespaceChars(0, ' ');


Why is 0 a whitespaceChar ?

Not only 0. Basically this line means that everything between 0 and will be treated as a whitespace character. This is to ignore all control characters below space including new line, line feed, tabs, new page etc. by treating it as a whitespace character. I have decided to leave it as is instead of configuring each a subset of character manually since this is the default behavior of StreamTokenizer, but we can be more restrictive here and allow only spaces, tabs and line feeds as suggested in the standard @iverase, @nknize what do you think?

The standard seems to only allow spaces, tabs or line feeds to improve readability so let's do as recommended.

imotov · 2019-01-11T14:42:16Z

@matriv I pushed the changes that you have requested. Could you take another look?

matriv

LGTM. Thank you for your patience.

karussell · 2019-01-21T15:54:52Z

I'm curious why not use JTS instead? (BSD-like license, actively maintained, solid implementation, wide spread adoption, without dependencies, ...)

karussell · 2019-01-21T15:56:32Z

Sorry, looks like this is answered in #35767:

JTS Geometry. This is what H2 is currently using. One problem with this solution is that we will have to require or ship JTS library and we will not be able to release GEOSQL until H2GIS 1.5.0 is released. We are using H2GIS for testing it and it is still using JTS 1.14 at the moment, the rest of ES is using JTS 1.15. So, if we cannot ship JDBC with JTS 1.14, but we cannot test it with JTS 1.15

nknize and others added 2 commits December 11, 2018 17:15

Add initial GeoShape hierachy

104d4e5

WIP

Geo: Clean up geo hierarchy

9b784e2

Removes all math and WKB parser and refactors WKT parser

imotov added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes v7.0.0 labels Dec 11, 2018

imotov requested review from iverase and nknize December 11, 2018 13:56

imotov changed the title ~~Adds a set of no dependency geo classes for JDBC driver~~ Geo: Adds a set of no dependency geo classes for JDBC driver Dec 11, 2018

This was referenced Dec 11, 2018

SQL: Adds a set of geo objects for JDBC driver #36415

Closed

SQL: representation of geo objects in JDBC #35767

Closed

alpar-t reviewed Dec 11, 2018

View reviewed changes

alpar-t added the :Delivery/Build Build or test infrastructure label Dec 11, 2018

alpar-t reviewed Dec 12, 2018

View reviewed changes

imotov added 2 commits December 12, 2018 14:28

Address @atorok's review comments

937e289

Merge remote-tracking branch 'elastic/master' into issue-35767-libs-geo

cde7964

alpar-t reviewed Dec 12, 2018

View reviewed changes

Address @atorok's 2nd review

01cae20

alpar-t reviewed Dec 13, 2018

View reviewed changes

Fix geo-test configuration for eclipse

a5acea2

alpar-t approved these changes Dec 17, 2018

View reviewed changes

imotov added 2 commits January 2, 2019 09:40

Move project name setting into settings.gradle

bb491d8

Merge remote-tracking branch 'elastic/master' into issue-35767-libs-geo

f2f0f55

iverase reviewed Jan 7, 2019

View reviewed changes

libs/geo/src/main/java/org/elasticsearch/geo/GeoUtils.java Show resolved Hide resolved

iverase reviewed Jan 7, 2019

View reviewed changes

libs/geo/src/main/java/org/elasticsearch/geo/geometry/GeometryVisitor.java Show resolved Hide resolved

imotov added 2 commits January 7, 2019 14:03

Add test for Geometry Visitor

7fdddb6

Make GeomUtils package private

f31d10c

Merge remote-tracking branch 'elastic/master' into issue-35767-libs-geo

168c947

nknize approved these changes Jan 8, 2019

View reviewed changes

imotov mentioned this pull request Jan 9, 2019

GEO: Switch to using GeoTestUtil to generate random geo shapes #37278

Closed

imotov added 2 commits January 9, 2019 15:32

Add javadoc with explanation of the Visitor Pattern

9408af7

Merge remote-tracking branch 'elastic/master' into issue-35767-libs-geo

a164360

matriv reviewed Jan 10, 2019

View reviewed changes

imotov added 3 commits January 10, 2019 08:54

Merge remote-tracking branch 'elastic/master' into issue-35767-libs-geo

524c6aa

Address review comments

4929333

Merge remote-tracking branch 'elastic/master' into issue-35767-libs-geo

ed3a92a

alpar-t mentioned this pull request Jan 11, 2019

Add SSL Configuration Library #37287

Merged

matriv approved these changes Jan 14, 2019

View reviewed changes

imotov merged commit 6f91f06 into elastic:master Jan 15, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

imotov mentioned this pull request Apr 5, 2019

Geo: Proposal for refactoring out ShapeBuilder hierarchy #40908

Closed

3 tasks

imotov deleted the issue-35767-libs-geo branch May 1, 2020 22:18

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

		public class LinearRing extends Line {
		public static final LinearRing EMPTY = new LinearRing();

		public static final MultiLine EMPTY = new MultiLine();

		private MultiLine() {

		public static final MultiPolygon EMPTY = new MultiPolygon();

		private MultiPolygon() {

Conversation

imotov commented Dec 11, 2018

Uh oh!

elasticmachine commented Dec 11, 2018

Uh oh!

alpar-t left a comment

Choose a reason for hiding this comment

Uh oh!

alpar-t Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Dec 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imotov commented Jan 2, 2019

Uh oh!

Uh oh!

Uh oh!

iverase commented Jan 8, 2019

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t Dec 11, 2018 •

edited

Loading