SQL: Add ST_WktToSQL function #35416

imotov · 2018-11-09T15:51:40Z

Adds support for ST_WktToSQL function which accepts a string and parses
it as WKT representation of a geoshape.

Relates to #29872

Adds support for ST_WktToSQL function which accepts a string and parses it as WKT representation of a geoshape. Relates to elastic#29872

elasticmachine · 2018-11-09T15:51:42Z

Pinging @elastic/es-search-aggs

matriv

Left some comments, but keep in mind I'm unfamiliar with the geo area...
Moreover, it would be nice to add a unit tests for these two cases:
https://github.com/elastic/elasticsearch/pull/35416/files#diff-758d3813c9c39f1bfd869ba1dcdfe045R39
https://github.com/elastic/elasticsearch/pull/35416/files#diff-758d3813c9c39f1bfd869ba1dcdfe045R44

For completeness I would add a test to QueryTranslatorTests to check the translsation to painless script.

matriv · 2018-11-11T22:11:40Z

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

+
+selectAsWKT
+// tag::wkttosql
+SELECT ST_AsWKT(ST_WKTToSQL('POINT (10 20)')) location;


Why do you have to wrap it with ST_AsWKT?

In this test we compare the result with csvjdbc result. The csvjdbc driver cannot deal with geoshapes. So, I have to convert the column into some type that csvjdbc understands.

matriv · 2018-11-11T22:12:05Z

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


     city:s    |     location:s
 Amsterdam      |point (4.850311987102032 52.347556999884546)
 // end::aswkt
 ;
+
+selectAsWKT


please fix test name (it's same as above)

matriv · 2018-11-11T22:14:35Z

...org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java

@@ -403,4 +405,8 @@ public static String aswktPoint(Object v) {
    public static String aswktShape(Object v) {
        return GeoProcessor.GeoOperation.ASWKT_SHAPE.apply(v).toString();
    }
+
+    public static GeoShape wktToSql(String v) {


minor: you could rename the param to wkt

matriv · 2018-11-11T22:16:30Z

...ain/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/StWkttosqlProcessor.java

+    public Object process(Object input) {
+        return StWkttosqlProcessor.apply(input);
+    }
+


minor: extra empty line

matriv · 2018-11-11T22:22:06Z

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

@@ -102,3 +102,32 @@ SELECT COUNT(city) count, CAST(SUBSTRING(ST_ASWKT(location), 8, 1) = '-' AS STRI
 9              |false
 6              |true
 ;
+
+selectFakeCoordinatesFromCityAndRegion
+SELECT region, city, ST_ASWKT(ST_WKTTOSQL(CONCAT(CONCAT(CONCAT(CONCAT('POINT (', CAST(LENGTH(city) AS STRING)), ' '), CAST(LENGTH(region) AS STRING)), ')'))) loc  FROM geo ORDER BY region, city;


I don't get why you need this complexity, but I'm also unfamiliar with area.
Isn't it better to add real coordinate date to the table (index) ?

Also again you wrap it with a call to ST_ASWKT, why is that? Shouldn't GeoShape implement a toString() method so that it returns a human readable string as requited?

Yes. It looks pretty ridiculous, I agree. I am going to significantly simplify it in the next iteration when I add some more useful functions that actually do something with shapes. So, I for now I need to work around lack of functions and inability to use cast because of #34557 and #33361

imotov · 2018-11-13T17:38:04Z

@matriv thanks for the review! I think I found a way to address your comments. Could you take another look?

imotov · 2018-11-13T17:48:13Z

retest this please

costin

LGTM

costin · 2018-11-13T20:53:38Z

...sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/StWkttosql.java

+/**
+ * Constructs geometric objects from their WTK representations
+ */
+public class StWkttosql extends UnaryScalarFunction {


It might be worth having a base Geo function that handles the validation and the return type so that subtypes only execute the actual action. If the pattern emerges, just passing the processor might be good enough (similar to the BinaryNumericFunction).
Just mentioning this for the future.

Yes, I did that for ST_AsWKT because I saw several functions that take Geometry as the only parameter. However, in this case ST_WKTToSQL is somewhat unique, so I have decided to wait until pattern emerges. I might need to refactor it, but I would like to add a few more functions first.

costin · 2018-11-13T20:58:15Z

x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt

+
+#### Classes
+
+class org.elasticsearch.xpack.sql.expression.function.scalar.geo.GeoShape {


Can we pass the native ES geo type (ShapeBuilder) directly or is there anything else SQL specific in there?

ShapeBuilder is using GeoJSON for toXContent() and pretty useless debugging output in toString(), which complicates the serialization story sinice we need to use WKT in both. At the moment this is the whole purpose of this wrapper. Basically I am trying to avoid adding a whole bunch of

if (obj instanceof ShapeBuilder) { ... use different serialization for ShapeBuilder ... } else { ... normal serialization for pretty much everything else ... }

I might also need to fold geo_point into this (not sure about this yet).

costin · 2018-11-13T21:01:26Z

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

+
+selectWKTToSQL
+// tag::wkttosql
+SELECT CAST(ST_WKTToSQL('POINT (10 20)') AS STRING) location;


we might be able to get away from this. For intervals, I've basically changed the way we do assertion where the actual type is checked and if it's of a custom type, a custom serialization takes place to check the strings.
In other words there would be no casting, CSV would read the result as a String but when the check is done, the geo type is detected, the result casted as the string and the two strings asserted.

No need to hold the PR for this.

Cool. I will definitely update this test when it lands in master.

matriv

LGTM.

imotov · 2018-11-15T04:37:14Z

retest this please

polyfractal · 2019-02-07T13:46:27Z

Bumping the version label to 7.1, since this merged into the geosql branch and that hasn't merged into master yet.

SQL: Add ST_WktToSQL function

024c079

Adds support for ST_WktToSQL function which accepts a string and parses it as WKT representation of a geoshape. Relates to elastic#29872

imotov added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes v7.0.0 :Analytics/SQL SQL querying labels Nov 9, 2018

imotov requested review from costin and astefan November 9, 2018 15:51

costin requested a review from matriv November 11, 2018 13:32

matriv reviewed Nov 11, 2018

View reviewed changes

imotov added 3 commits November 13, 2018 10:42

Address review comments and add some JDBC tests

ed0aa6c

Cast geoshape in docs instead of using ST_AsWKT

68c6fa0

Add QueryTranslatorTests

3be14f9

costin approved these changes Nov 13, 2018

View reviewed changes

imotov added 2 commits November 14, 2018 14:46

Merge branch 'geosql' into geosql-add-geometry-from-text

ff2209c

Merge branch 'geosql' into geosql-add-geometry-from-text

cb8c136

matriv reviewed Nov 14, 2018

View reviewed changes

imotov merged commit 08da5bb into elastic:geosql Nov 15, 2018

tomcallahan removed the :Analytics/SQL SQL querying label Dec 18, 2018

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

$@polyfractal$ polyfractal added v7.2.0 and removed v7.0.0-beta1 labels Feb 7, 2019

imotov deleted the geosql-add-geometry-from-text branch May 1, 2020 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Add ST_WktToSQL function #35416

SQL: Add ST_WktToSQL function #35416

imotov commented Nov 9, 2018

elasticmachine commented Nov 9, 2018

matriv left a comment

matriv Nov 11, 2018

imotov Nov 12, 2018

matriv Nov 11, 2018

matriv Nov 11, 2018

matriv Nov 11, 2018

matriv Nov 11, 2018

imotov Nov 12, 2018

imotov commented Nov 13, 2018

imotov commented Nov 13, 2018

costin left a comment

costin Nov 13, 2018

imotov Nov 14, 2018

costin Nov 13, 2018

imotov Nov 14, 2018

costin Nov 13, 2018

imotov Nov 14, 2018

matriv left a comment

imotov commented Nov 15, 2018

polyfractal commented Feb 7, 2019


		#### Classes

		class org.elasticsearch.xpack.sql.expression.function.scalar.geo.GeoShape {

SQL: Add ST_WktToSQL function #35416

SQL: Add ST_WktToSQL function #35416

Conversation

imotov commented Nov 9, 2018

elasticmachine commented Nov 9, 2018

matriv 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

imotov commented Nov 13, 2018

imotov commented Nov 13, 2018

costin 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

matriv left a comment

Choose a reason for hiding this comment

imotov commented Nov 15, 2018

polyfractal commented Feb 7, 2019