SQL: Switch JDBC driver to libs/geo objects for geo_shapes #39204

imotov · 2019-02-20T20:49:30Z

Switches JDBC driver to use libs/geo objects when server returns a
geo_shape. This PR doesn't address the geo_points yet, which will be
addressed in a separate PR.

Switches JDBC driver to use libs/geo objects when server returns a geo_shape. This PR doesn't address the geo_points yet, which will be addressed in a separate PR.

elasticmachine · 2019-02-20T20:49:32Z

Pinging @elastic/es-analytics-geo

elasticmachine · 2019-02-20T20:49:33Z

Pinging @elastic/es-search

matriv

LGTM. Left a minor comment.

matriv · 2019-02-21T12:38:01Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcAssert.java

@@ -269,11 +268,11 @@ else if (type == Types.DOUBLE) {
                    } else if (type == Types.FLOAT) {
                        assertEquals(msg, (float) expectedObject, (float) actualObject, lenientFloatingNumbers ? 1f : 0.0f);
                    } else if (type == Types.OTHER) {
-                        if (expectedObject instanceof Geometry && actualObject instanceof String) {
+                        if (actualObject instanceof org.elasticsearch.geo.geometry.Geometry) {


Please import to avoid the qualifier.

@bpintea can you please advice what type the ODBC driver indicates (if at all) for geo?

Own update: it looks like both geometry and geography are mapped to -151.

@bpintea can you please advice what type the ODBC driver indicates (if at all) for geo?

Own update: it looks like both geometry and geography are mapped to -151.

@costin There isn't any specification on how the geo types should be conveyed in ODBC. Most DBMSes use private types/extensions (like MS SQL's "user defined" -151). WKT/B seems to have most support (also among some BI tools and Excel plug-ins), with GeoJSON picking up as well.

An idea would be to apply the same logic as with KEYWORD, TEXT and IP: give the new geography/geometry types respective names, but reuse a character (12) or the binary (-3) type.
Since Geo is not part of the ODBC standard, no 3rd party existing app could "out of the box" make use of whatever data types we'll implement. OTOH, the values encoding (WKT/B) could be used.

costin

LGTM

costin · 2019-02-24T21:12:56Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcAssert.java

@@ -269,11 +268,11 @@ else if (type == Types.DOUBLE) {
                    } else if (type == Types.FLOAT) {
                        assertEquals(msg, (float) expectedObject, (float) actualObject, lenientFloatingNumbers ? 1f : 0.0f);
                    } else if (type == Types.OTHER) {
-                        if (expectedObject instanceof Geometry && actualObject instanceof String) {
+                        if (actualObject instanceof org.elasticsearch.geo.geometry.Geometry) {


@bpintea can you please advice what type the ODBC driver indicates (if at all) for geo?

Own update: it looks like both geometry and geography are mapped to -151.

astefan

LGTM. Left some minor comments.

astefan · 2019-02-25T14:54:28Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcAssert.java

@@ -269,11 +268,11 @@ else if (type == Types.DOUBLE) {
                    } else if (type == Types.FLOAT) {
                        assertEquals(msg, (float) expectedObject, (float) actualObject, lenientFloatingNumbers ? 1f : 0.0f);
                    } else if (type == Types.OTHER) {
-                        if (expectedObject instanceof Geometry && actualObject instanceof String) {
+                        if (actualObject instanceof org.elasticsearch.geo.geometry.Geometry) {
                            // We need to convert the actual object to Geometry for comparision


Here your comment mentions the actual object needing conversion, but two lines below you perform the operation for the "expected" value instead. Shouldn't be the "actual" one conversion attempt?

The comment is leftover from the previous implementation. Before we had string in actual and JTS Geometry in expected. With this change I am trying to reduce dependency on JTS and H2 in this code, so instead of converting everything to H2's JTS Geometry I am converting it to our geometry. I will update the comment. Good catch!

astefan · 2019-02-25T14:55:55Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/geo/GeoSqlSpecTestCase.java

    });

    @ParametersFactory(argumentFormatting = PARAM_FORMATTING)
    public static List<Object[]> readScriptSpec() throws Exception {
        Parser parser = new SqlSpecParser();
        List<Object[]> tests = new ArrayList<>();
+        tests.addAll(readScriptSpec("/geo/geosql.sql-spec", parser));


Picky comment: can you put the geo script file addition after "ogc"? (this is the order in the other two changes in this class - line 37 and line 56)

astefan · 2019-02-25T14:57:57Z

x-pack/plugin/sql/qa/src/main/resources/geo/geosql.sql-spec

+selectRegionUsingWktToSqlWithoutConvertion
+SELECT region, city, shape, ST_GEOMFROMTEXT(region_point) region_wkt  FROM geo ORDER BY region, city;
+
+// TODO: the ordering of geometries differs so ORDER BY needs to be wrapped into string for now


I would create an issue for this TODO, just not to lose track of it. If there is one already, apologies.

It looks like order of the elements is not defined by the standard. So, I will order by something else in this test and remove the TODO.

astefan · 2019-02-25T14:59:10Z

x-pack/plugin/sql/qa/src/main/resources/geo/geosql.sql-spec

+
+// TODO: the ordering of geometries differs so ORDER BY needs to be wrapped into string for now
+selectCitiesWithAGroupByWktToSql
+SELECT COUNT(city) city_by_region, ST_GEOMFROMTEXT(region_point) region FROM geo WHERE city LIKE '%a%' GROUP BY ST_GEOMFROMTEXT(region_point) ORDER BY ST_ASWKT(ST_GEOMFROMTEXT(region_point));


I would add a test with a group by alias: SELECT COUNT(city) city_by_region, ST_GEOMFROMTEXT(region_point) region FROM geo GROUP BY region.

astefan · 2019-02-25T14:59:35Z

x-pack/plugin/sql/qa/src/main/resources/geo/geosql.sql-spec

+SELECT COUNT(city) city_by_region, ST_GEOMFROMTEXT(region_point) region FROM geo WHERE city LIKE '%a%' GROUP BY ST_GEOMFROMTEXT(region_point) ORDER BY ST_ASWKT(ST_GEOMFROMTEXT(region_point));
+
+selectCitiesWithEOrderByWktToSql
+SELECT region, city FROM geo WHERE city LIKE '%e%' ORDER BY ST_ASWKT(ST_GEOMFROMTEXT(region_point)), city;


Same as above - a query with ORDER BY aliased geo function.

astefan · 2019-02-25T15:00:25Z

x-pack/plugin/sql/qa/src/main/resources/geo/setup_test_geo.sql

+                    "region" VARCHAR(50),
+                    "region_point" VARCHAR(50),
+                    "shape" GEOMETRY
+


Remove empty line?

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

5f9cca7

Switches JDBC driver to use libs/geo objects when server returns a geo_shape. This PR doesn't address the geo_points yet, which will be addressed in a separate PR.

imotov added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes :Analytics/SQL SQL querying labels Feb 20, 2019

imotov requested review from astefan and matriv February 20, 2019 20:49

matriv approved these changes Feb 21, 2019

View reviewed changes

costin approved these changes Feb 24, 2019

View reviewed changes

astefan approved these changes Feb 25, 2019

View reviewed changes

imotov added 3 commits February 26, 2019 11:48

Merge branch 'geosql' into issue-35767-integrate-libsgeo

a35fdff

Address review comments

dc8fb16

Fix documentation

7752de5

imotov merged commit 44542b0 into elastic:geosql Feb 27, 2019

bpintea mentioned this pull request Mar 1, 2019

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

Merged

imotov deleted the issue-35767-integrate-libsgeo branch May 1, 2020 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Switch JDBC driver to libs/geo objects for geo_shapes #39204

SQL: Switch JDBC driver to libs/geo objects for geo_shapes #39204

imotov commented Feb 20, 2019

elasticmachine commented Feb 20, 2019

elasticmachine commented Feb 20, 2019

matriv left a comment

matriv Feb 21, 2019

costin Feb 24, 2019 •

edited

Loading

bpintea Mar 1, 2019

costin left a comment

costin Feb 24, 2019 •

edited

Loading

astefan left a comment

astefan Feb 25, 2019

imotov Feb 26, 2019

astefan Feb 25, 2019

astefan Feb 25, 2019

imotov Feb 26, 2019

astefan Feb 25, 2019

astefan Feb 25, 2019

astefan Feb 25, 2019

SQL: Switch JDBC driver to libs/geo objects for geo_shapes #39204

SQL: Switch JDBC driver to libs/geo objects for geo_shapes #39204

Conversation

imotov commented Feb 20, 2019

elasticmachine commented Feb 20, 2019

elasticmachine commented Feb 20, 2019

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin Feb 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

costin Feb 24, 2019 • edited Loading

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin Feb 24, 2019 •

edited

Loading

costin Feb 24, 2019 •

edited

Loading