SQL: Add ST_X, ST_Y and ST_GEOMETRY_TYPE functions #41104
Conversation
Adds 3 new geosql functions. The ST_Z functions as well as ST_X and ST_Y functions for polygons are not supported because of the issues described in elastic#40908. The suppor will be added as soon as the ShapeBuilder issues are resolved. Relates to elastic#29872
imotov
added
>enhancement
:Analytics/Geo
|
Pinging @elastic/es-analytics-geo |
|
Pinging @elastic/es-search |
|
|
||
| .Description: | ||
|
|
||
| Returns the type of the `geometry` such as "Point", "Polygon", "MultiPolygon", etc. |
There was a problem hiding this comment.
Can you list all of the possibilities (if they are not many), or provide a link to a list of types?
|
|
||
| ["source","sql",subs="attributes,macros"] | ||
| -------------------------------------------------- | ||
| include-tagged::{sql-specs}/geo/docs.csv-spec[geometrytype] |
There was a problem hiding this comment.
At the moment, on master, the docs tests and samples go into a /docs/docs.csv-spec. Any chance this will go there when the merge with master happens? Or maybe put the geo ones inside a /docs/docs.csv-spec file, so that you don't forget at merging time?
There was a problem hiding this comment.
Maybe it makes sense to have them under /docs/geo.csv-spec but be sure that the file is picked up from our testing framework.
There was a problem hiding this comment.
@costin asked me earlier to separate geo-related things into a separate directory as much as possible. I am just following this suggestion. I think moving it into /docs/geo.csv-spec might be a good compromise here.
| @@ -137,6 +137,9 @@ | |||
| * <<sql-functions-geo>> | |||
| ** <<sql-functions-geo-st-as-wkt>> | |||
| ** <<sql-functions-geo-st-wkt-to-sql>> | |||
| ** <<sql-functions-geo-st-geometrytype>> | |||
There was a problem hiding this comment.
I think this one has to go before wkt-to-sql? (they should be listed alphabetically for ease of browsing)
| @@ -214,3 +214,55 @@ SELECT ST_Distance(ST_WktToSql(NULL), ST_WktToSQL('POINT (-71 42)')) shape; | |||
| shape:d | |||
| null | |||
| ; | |||
|
|
|||
| groupByGeometryType | |||
| SELECT COUNT(*) cnt, ST_GeometryType(location) gt FROM geo GROUP BY ST_GeometryType(location); | |||
There was a problem hiding this comment.
Can you also add a test where the alias gt is used as GROUP BY and ORDER BY?
| selectFilterByXOfLocation | ||
| SELECT city, ST_X(shape) x, ST_Y(shape) y FROM geo WHERE ST_X(location) > 0 ORDER BY ST_Y(location); | ||
|
|
||
| city:s | x:d | y:d |
There was a problem hiding this comment.
In this query I would also add as returned columns ST_X(location) and ST_Y(location) just to be easier to double check the results' correctness.
| selectFilterByRegionPoint | ||
| SELECT city, region FROM geo WHERE ST_X(ST_WKTTOSQL(region_point)) < 0 ORDER BY ST_X(location); | ||
|
|
||
| city:s | region:s |
There was a problem hiding this comment.
Same here: I would add ST_X(location) in the returned results to double check the correctness of the results.
| ASWKT(GeoShape::toString), | ||
| X(GeoShape::getX), | ||
| Y(GeoShape::getY), | ||
| GEOMETRY_TYPE(GeoShape::getGeometryType); |
There was a problem hiding this comment.
Can you put this one second in line, please? (considering an alphabetic ordering)
|
|
||
| public Double getX() { | ||
| Point firstPoint = firstPoint(); | ||
| if (firstPoint != null) { |
There was a problem hiding this comment.
All this logic could be re-written in a more compact way as return firstPoint != null ? firstPoint.getLon() : null;
|
|
||
| public Double getY() { | ||
| Point firstPoint = firstPoint(); | ||
| if (firstPoint != null) { |
There was a problem hiding this comment.
Same here about a more compact way of returning a value.
| @@ -150,6 +150,9 @@ class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalS | |||
| String stAswkt(Object) | |||
| GeoShape stWktToSql(String) | |||
There was a problem hiding this comment.
I would order these alphabetically withing the "geo" section.
|
|
||
| @Override | ||
| public Point visit(MultiLine multiLine) { | ||
| return visit((GeometryCollection<?>) multiLine); |
There was a problem hiding this comment.
I think here and below the cast is unnecessary?
There was a problem hiding this comment.
Without this cast it will go into an infinite loop.
|
@elasticmachine run elasticsearch-ci/1 |
Adds 3 new geosql functions. The ST_Z functions as well as ST_X and
ST_Y functions for polygons are not supported because of the issues
described in #40908. This support will be added as soon as the
ShapeBuilder issues are resolved.
Relates to #29872