SQL: Switch JDBC driver to libs/geo objects for geo_points

Switches JDBC driver to use libs/geo objects when server returns a geo_point. Closes elastic#35767
imotov · Feb 27, 2019 · 8308223 · 8308223
1 parent 787eef6
commit 8308223
Show file tree

Hide file tree

Showing 14 changed files with 82 additions and 91 deletions.
diff --git a/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeConverter.java b/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeConverter.java
@@ -237,7 +237,6 @@ static Object convert(Object v, EsType columnType, String typeString) throws SQL
             case INTERVAL_MINUTE_TO_SECOND:
                 return Duration.parse(v.toString());
             case GEO_POINT:
-                return v;
             case GEO_SHAPE:
                 try {
                     return WellKnownText.fromWKT(v.toString());

diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcAssert.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcAssert.java
@@ -10,6 +10,7 @@
 
 import org.apache.logging.log4j.Logger;
 import org.elasticsearch.geo.geometry.Geometry;
+import org.elasticsearch.geo.geometry.Point;
 import org.elasticsearch.geo.utils.WellKnownText;
 import org.elasticsearch.xpack.sql.jdbc.EsType;
 import org.elasticsearch.xpack.sql.proto.StringUtils;
@@ -36,6 +37,8 @@
 import static java.sql.Types.REAL;
 import static java.sql.Types.SMALLINT;
 import static java.sql.Types.TINYINT;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -277,8 +280,15 @@ else if (type == Types.DOUBLE) {
                                 fail(ex.getMessage());
                             }
                         }
-                        // TODO: For now we will just do toString()-based comparision
-                        assertEquals(msg, expectedObject, actualObject);
+                        if (actualObject instanceof Point) {
+                            // geo points are loaded form doc values where they are stored as long-encoded values leading
+                            // to lose in precision
+                            assertThat(expectedObject, instanceOf(Point.class));
+                            assertEquals(((Point) expectedObject).getLat(), ((Point) actualObject).getLat(), 0.000001d);
+                            assertEquals(((Point) expectedObject).getLon(), ((Point) actualObject).getLon(), 0.000001d);
+                        } else {
+                            assertEquals(msg, expectedObject, actualObject);
+                        }
                     }
                     // intervals
                     else if (type == Types.VARCHAR && actualObject instanceof TemporalAmount) {

diff --git a/x-pack/plugin/sql/qa/src/main/resources/geo/geo.csv b/x-pack/plugin/sql/qa/src/main/resources/geo/geo.csv
@@ -1,16 +1,16 @@
-city,region,region_point,shape
-Mountain View,Americas,POINT(-105.2551 54.5260),point (-122.083843 37.386483)
-Chicago,Americas,POINT(-105.2551 54.5260),point (-87.637874 41.888783)
-New York,Americas,POINT(-105.2551 54.5260),point (-73.990027 40.745171)
-San Francisco,Americas,POINT(-105.2551 54.5260),point (-122.394228 37.789541)
-Phoenix,Americas,POINT(-105.2551 54.5260),point (-111.973505 33.376242)
-Amsterdam,Europe,POINT(15.2551 54.5260),point (4.850312 52.347557)
-Berlin,Europe,POINT(15.2551 54.5260),point (13.390889 52.486701)
-Munich,Europe,POINT(15.2551 54.5260),point (11.537505 48.146321)
-London,Europe,POINT(15.2551 54.5260),point (-0.121672 51.510871)
-Paris,Europe,POINT(15.2551 54.5260),point (2.351773 48.845538)
-Singapore,Asia,POINT(100.6197 34.0479),point (103.855535 1.295868)
-Hong Kong,Asia,POINT(100.6197 34.0479),point (114.183925 22.281397)
-Seoul,Asia,POINT(100.6197 34.0479),point (127.060851 37.509132)
-Tokyo,Asia,POINT(100.6197 34.0479),point (139.76402225 35.669616)
-Sydney,Asia,POINT(100.6197 34.0479),point (151.208629 -33.863385)
+city,region,region_point,location,shape
+Mountain View,Americas,POINT(-105.2551 54.5260),point (-122.083843 37.386483),point (-122.083843 37.386483)
+Chicago,Americas,POINT(-105.2551 54.5260),point (-87.637874 41.888783),point (-87.637874 41.888783)
+New York,Americas,POINT(-105.2551 54.5260),point (-73.990027 40.745171),point (-73.990027 40.745171)
+San Francisco,Americas,POINT(-105.2551 54.5260),point (-122.394228 37.789541),point (-122.394228 37.789541)
+Phoenix,Americas,POINT(-105.2551 54.5260),point (-111.973505 33.376242),point (-111.973505 33.376242)
+Amsterdam,Europe,POINT(15.2551 54.5260),point (4.850312 52.347557),point (4.850312 52.347557)
+Berlin,Europe,POINT(15.2551 54.5260),point (13.390889 52.486701),point (13.390889 52.486701)
+Munich,Europe,POINT(15.2551 54.5260),point (11.537505 48.146321),point (11.537505 48.146321)
+London,Europe,POINT(15.2551 54.5260),point (-0.121672 51.510871),point (-0.121672 51.510871)
+Paris,Europe,POINT(15.2551 54.5260),point (2.351773 48.845538),point (2.351773 48.845538)
+Singapore,Asia,POINT(100.6197 34.0479),point (103.855535 1.295868),point (103.855535 1.295868)
+Hong Kong,Asia,POINT(100.6197 34.0479),point (114.183925 22.281397),point (114.183925 22.281397)
+Seoul,Asia,POINT(100.6197 34.0479),point (127.060851 37.509132),point (127.060851 37.509132)
+Tokyo,Asia,POINT(100.6197 34.0479),point (139.76402225 35.669616),point (139.76402225 35.669616)
+Sydney,Asia,POINT(100.6197 34.0479),point (151.208629 -33.863385),point (151.208629 -33.863385)
diff --git a/x-pack/plugin/sql/qa/src/main/resources/geo/geosql.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/geo/geosql.csv-spec
@@ -28,22 +28,22 @@ shape                | GEOMETRY   | geo_shape
 selectAllPointsAsStrings
 SELECT city, CAST(location AS STRING) location, CAST(shape AS STRING) shape, region FROM "geo" ORDER BY "city";
 
-     city:s    |      location:s                       |             shape:s          |  region:s
-Amsterdam      |52.347556999884546, 4.850311987102032  |point (4.850312 52.347557)    |Europe
-Berlin         |52.48670099303126, 13.390888944268227  |point (13.390889 52.486701)   |Europe
-Chicago        |41.888782968744636, -87.63787407428026 |point (-87.637874 41.888783)  |Americas
-Hong Kong      |22.28139698971063, 114.18392493389547  |point (114.183925 22.281397)  |Asia
-London         |51.51087098289281, -0.12167204171419144|point (-0.121672 51.510871)   |Europe
-Mountain View  |37.38648299127817, -122.08384302444756 |point (-122.083843 37.386483) |Americas
-Munich         |48.14632098656148, 11.537504978477955  |point (11.537505 48.146321)   |Europe
-New York       |40.74517097789794, -73.9900270756334   |point (-73.990027 40.745171)  |Americas
-Paris          |48.84553796611726, 2.3517729341983795  |point (2.351773 48.845538)    |Europe
-Phoenix        |33.37624196894467, -111.97350500151515 |point (-111.973505 33.376242) |Americas
-San Francisco  |37.789540970698, -122.39422800019383   |point (-122.394228 37.789541) |Americas
-Seoul          |37.50913198571652, 127.06085099838674  |point (127.060851 37.509132)  |Asia
-Singapore      |1.2958679627627134, 103.8555349688977  |point (103.855535 1.295868)   |Asia
-Sydney         |-33.863385021686554, 151.20862897485495|point (151.208629 -33.863385) |Asia
-Tokyo          |35.66961596254259, 139.76402222178876  |point (139.76402225 35.669616)|Asia
+     city:s    |             location:s                       |             shape:s          |  region:s
+Amsterdam      |point (4.850311987102032 52.347556999884546)  |point (4.850312 52.347557)    |Europe
+Berlin         |point (13.390888944268227 52.48670099303126)  |point (13.390889 52.486701)   |Europe
+Chicago        |point (-87.63787407428026 41.888782968744636) |point (-87.637874 41.888783)  |Americas
+Hong Kong      |point (114.18392493389547 22.28139698971063)  |point (114.183925 22.281397)  |Asia
+London         |point (-0.12167204171419144 51.51087098289281)|point (-0.121672 51.510871)   |Europe
+Mountain View  |point (-122.08384302444756 37.38648299127817) |point (-122.083843 37.386483) |Americas
+Munich         |point (11.537504978477955 48.14632098656148)  |point (11.537505 48.146321)   |Europe
+New York       |point (-73.9900270756334 40.74517097789794)   |point (-73.990027 40.745171)  |Americas
+Paris          |point (2.3517729341983795 48.84553796611726)  |point (2.351773 48.845538)    |Europe
+Phoenix        |point (-111.97350500151515 33.37624196894467) |point (-111.973505 33.376242) |Americas
+San Francisco  |point (-122.39422800019383 37.789540970698)   |point (-122.394228 37.789541) |Americas
+Seoul          |point (127.06085099838674 37.50913198571652)  |point (127.060851 37.509132)  |Asia
+Singapore      |point (103.8555349688977 1.2958679627627134)  |point (103.855535 1.295868)   |Asia
+Sydney         |point (151.20862897485495 -33.863385021686554)|point (151.208629 -33.863385) |Asia
+Tokyo          |point (139.76402222178876 35.66961596254259)  |point (139.76402225 35.669616)|Asia
 ;
 
 // TODO: Both shape and location contain the same data for now, we should change it later to make things more interesting

diff --git a/x-pack/plugin/sql/qa/src/main/resources/geo/geosql.sql-spec b/x-pack/plugin/sql/qa/src/main/resources/geo/geosql.sql-spec
@@ -2,12 +2,18 @@
 // Commands on geo test data
 //
 
-selectAllPointsAsStrings
+selectAllShapesAsGeometries
 SELECT city, shape, region FROM "geo" ORDER BY "city";
 
-selectAllPointsAsWKT
+selectAllShapesAsWKT
 SELECT city, ST_GEOMFROMTEXT(ST_ASWKT(shape)) shape_wkt, region FROM "geo" ORDER BY "city";
 
+selectAllPointsAsGeometries
+SELECT city, location, region FROM "geo" ORDER BY "city";
+
+selectAllPointsAsWKT
+SELECT city, ST_GEOMFROMTEXT(ST_ASWKT(location)) shape_wkt, region FROM "geo" ORDER BY "city";
+
 selectRegionUsingWktToSqlWithoutConvertion
 SELECT region, city, shape, ST_GEOMFROMTEXT(region_point) region_wkt  FROM geo ORDER BY region, city;
 

diff --git a/x-pack/plugin/sql/qa/src/main/resources/geo/setup_test_geo.sql b/x-pack/plugin/sql/qa/src/main/resources/geo/setup_test_geo.sql
@@ -3,6 +3,7 @@ CREATE TABLE "geo" (
                     "city" VARCHAR(50),
                     "region" VARCHAR(50),
                     "region_point" VARCHAR(50),
+                    "location" POINT,
                     "shape" GEOMETRY
 )
   AS SELECT * FROM CSVREAD('classpath:/geo/geo.csv');
diff --git a/...c/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java b/...c/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java
@@ -9,6 +9,7 @@
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.document.DocumentField;
+import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.geo.GeoUtils;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
@@ -123,11 +124,11 @@ private Object unwrapMultiValue(Object values) {
                 value = values;
             }
             try {
-                return GeoUtils.parseGeoPoint(value, true);
+                GeoPoint geoPoint = GeoUtils.parseGeoPoint(value, true);
+                return new GeoShape(geoPoint.lon(), geoPoint.lat());
             } catch (ElasticsearchParseException ex) {
                 throw new SqlIllegalArgumentException("Cannot parse geo_point value (returned by [{}])", fieldName);
             }
-
         }
         if (dataType == DataType.GEO_SHAPE) {
             Object value;

diff --git a/...rc/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoProcessor.java b/...rc/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoProcessor.java
@@ -6,7 +6,6 @@
 package org.elasticsearch.xpack.sql.expression.function.scalar.geo;
 
 import org.elasticsearch.common.geo.GeoPoint;
-import org.elasticsearch.common.geo.builders.PointBuilder;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
@@ -17,40 +16,25 @@
 
 public class GeoProcessor implements Processor {
 
-    private interface GeoPointFunction<R> {
-        default R apply(Object o) {
-            if (!(o instanceof GeoPoint)) {
-                throw new SqlIllegalArgumentException("A geo_point is required; received [{}]", o);
-            }
-            return doApply((GeoPoint) o);
-        }
-
-        R doApply(GeoPoint s);
-    }
-
-
     private interface GeoShapeFunction<R> {
         default R apply(Object o) {
-            if (!(o instanceof GeoShape)) {
-                throw new SqlIllegalArgumentException("A geo_shape is required; received [{}]", o);
+            if (o instanceof GeoPoint) {
+                return doApply(new GeoShape(((GeoPoint) o).getLon(), ((GeoPoint) o).getLat()));
+            } else if (o instanceof GeoShape) {
+                return doApply((GeoShape) o);
+            } else {
+                throw new SqlIllegalArgumentException("A geo_point or geo_shape is required; received [{}]", o);
             }
-
-            return doApply((GeoShape) o);
         }
 
         R doApply(GeoShape s);
     }
 
     public enum GeoOperation {
-        ASWKT_POINT((GeoPoint p) -> new PointBuilder(p.getLon(), p.getLat()).toWKT()),
-        ASWKT_SHAPE(GeoShape::toString);
+        ASWKT(GeoShape::toString);
 
         private final Function<Object, Object> apply;
 
-        GeoOperation(GeoPointFunction<Object> apply) {
-            this.apply = l -> l == null ? null : apply.apply(l);
-        }
-
         GeoOperation(GeoShapeFunction<Object> apply) {
             this.apply = l -> l == null ? null : apply.apply(l);
         }

diff --git a/...ql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoShape.java b/...ql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoShape.java
@@ -5,6 +5,7 @@
  */
 package org.elasticsearch.xpack.sql.expression.function.scalar.geo;
 
+import org.elasticsearch.common.geo.builders.PointBuilder;
 import org.elasticsearch.common.geo.builders.ShapeBuilder;
 import org.elasticsearch.common.geo.parsers.ShapeParser;
 import org.elasticsearch.common.xcontent.ToXContentFragment;
@@ -22,6 +23,10 @@ public class GeoShape implements ToXContentFragment {
 
     private final ShapeBuilder<?, ?, ?> shapeBuilder;
 
+    public GeoShape(double lon, double lat) {
+        shapeBuilder = new PointBuilder(lon, lat);
+    }
+
     public GeoShape(Object value) throws IOException {
         shapeBuilder = ShapeParser.parse(value);
     }

diff --git a/...sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/StAswkt.java b/...sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/StAswkt.java
@@ -34,11 +34,7 @@ protected StAswkt replaceChild(Expression newChild) {
 
     @Override
     protected GeoOperation operation() {
-        if (field().dataType() == DataType.GEO_POINT) {
-            return GeoOperation.ASWKT_POINT;
-        } else {
-            return GeoOperation.ASWKT_SHAPE;
-        }
+        return GeoOperation.ASWKT;
     }
 
     @Override

diff --git a/.../elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java b/.../elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java
@@ -474,12 +474,8 @@ public static String ucase(String s) {
         return (String) StringOperation.UCASE.apply(s);
     }
 
-    public static String aswktPoint(Object v) {
-        return GeoProcessor.GeoOperation.ASWKT_POINT.apply(v).toString();
-    }
-
-    public static String aswktShape(Object v) {
-        return GeoProcessor.GeoOperation.ASWKT_SHAPE.apply(v).toString();
+    public static String aswkt(Object v) {
+        return GeoProcessor.GeoOperation.ASWKT.apply(v).toString();
     }
 
     public static GeoShape wktToSql(String wktString) {

diff --git a/x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt b/x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt
@@ -137,8 +137,7 @@ class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalS
   String  space(Number)
   String  substring(String, Number, Number)
   String  ucase(String)
-  String aswktPoint(Object)
-  String aswktShape(Object)
+  String  aswkt(Object)
   GeoShape wktToSql(String)
 
 #

diff --git a/...st/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoProcessorTests.java b/...st/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoProcessorTests.java
@@ -5,14 +5,11 @@
  */
 package org.elasticsearch.xpack.sql.expression.function.scalar.geo;
 
-import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.io.stream.Writeable.Reader;
 import org.elasticsearch.test.AbstractWireSerializingTestCase;
 import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 import org.elasticsearch.xpack.sql.expression.function.scalar.geo.GeoProcessor.GeoOperation;
 
-import java.io.IOException;
-
 public class GeoProcessorTests extends AbstractWireSerializingTestCase<GeoProcessor> {
     public static GeoProcessor randomGeoProcessor() {
         return new GeoProcessor(randomFrom(GeoOperation.values()));
@@ -28,28 +25,25 @@ protected Reader<GeoProcessor> instanceReader() {
         return GeoProcessor::new;
     }
 
-    @Override
-    protected GeoProcessor mutateInstance(GeoProcessor instance) throws IOException {
-        return new GeoProcessor(randomValueOtherThan(instance.processor(), () -> randomFrom(GeoOperation.values())));
-    }
+    //TODO: Restore mutateInstance when we have more GeoOperations
 
     public void testApply() throws Exception {
-        GeoProcessor proc = new GeoProcessor(GeoOperation.ASWKT_POINT);
+        GeoProcessor proc = new GeoProcessor(GeoOperation.ASWKT);
         assertNull(proc.process(null));
-        assertEquals("point (10.0 20.0)", proc.process(new GeoPoint(20, 10)));
+        assertEquals("point (10.0 20.0)", proc.process(new GeoShape(10, 20)));
 
-        proc = new GeoProcessor(GeoOperation.ASWKT_SHAPE);
+        proc = new GeoProcessor(GeoOperation.ASWKT);
         assertNull(proc.process(null));
         assertEquals("point (10.0 20.0)", proc.process(new GeoShape("POINT (10 20)")));
     }
 
     public void testTypeCheck() {
-        GeoProcessor procPoint = new GeoProcessor(GeoOperation.ASWKT_POINT);
+        GeoProcessor procPoint = new GeoProcessor(GeoOperation.ASWKT);
         SqlIllegalArgumentException siae = expectThrows(SqlIllegalArgumentException.class, () -> procPoint.process("string"));
-        assertEquals("A geo_point is required; received [string]", siae.getMessage());
+        assertEquals("A geo_point or geo_shape is required; received [string]", siae.getMessage());
 
-        GeoProcessor procShape = new GeoProcessor(GeoOperation.ASWKT_SHAPE);
+        GeoProcessor procShape = new GeoProcessor(GeoOperation.ASWKT);
         siae = expectThrows(SqlIllegalArgumentException.class, () -> procShape.process("string"));
-        assertEquals("A geo_shape is required; received [string]", siae.getMessage());
+        assertEquals("A geo_point or geo_shape is required; received [string]", siae.getMessage());
     }
 }
diff --git a/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java
@@ -456,7 +456,7 @@ public void testTranslateStAsWktForPoints() {
         assertNull(translation.query);
         AggFilter aggFilter = translation.aggFilter;
         assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.eq(" +
-                "InternalSqlScriptUtils.aswktPoint(InternalSqlScriptUtils.docValue(doc,params.v0))," +
+                "InternalSqlScriptUtils.aswkt(InternalSqlScriptUtils.docValue(doc,params.v0))," +
                 "params.v1)" +
                 ")",
             aggFilter.scriptTemplate().toString());
@@ -474,7 +474,7 @@ public void testTranslateStAsWktForShapes() {
         assertNull(translation.query);
         AggFilter aggFilter = translation.aggFilter;
         assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.eq(" +
-                "InternalSqlScriptUtils.aswktShape(InternalSqlScriptUtils.docValue(doc,params.v0))," +
+                "InternalSqlScriptUtils.aswkt(InternalSqlScriptUtils.docValue(doc,params.v0))," +
                 "params.v1)" +
                 ")",
             aggFilter.scriptTemplate().toString());