SQL: Add ST_Distance function to geosql #39973
Conversation
Adds ST_Distance function that works on points. No other shapes are supported at the moments. No optimization and conversion into geo_distance filter is done yet. Relates to elastic#29872
imotov
added
>enhancement
:Analytics/Geo
|
Pinging @elastic/es-analytics-geo |
|
Pinging @elastic/es-search |
|
|
||
| selectDistance | ||
| // tag::distance | ||
| SELECT ST_Distance(ST_WKTToSQL('POINT (10 20)'), ST_WKTToSQL('POINT (20 30)')) distance; |
There was a problem hiding this comment.
Please add a query with rounding as well (to the nearest m I suppose).
|
|
||
| @Override | ||
| protected String scriptMethodName() { | ||
| return "distance"; |
There was a problem hiding this comment.
Maybe add the st prefix to avoid potential future name clashes with another 'distance' method?
| } | ||
|
|
||
| // processes doc value as a geometry | ||
| public static <T> GeoShape geoDocValue(Map<String, ScriptDocValues<T>> doc, String fieldName) { |
There was a problem hiding this comment.
For the future the plan is to move away from docvalues and use source instead. Would that work for geo or not? In other words, are the normalized doc values needed (like it is for dates due to formatting) or is the input already normalized (e.g. numbers)?
There was a problem hiding this comment.
We can do it for hit extractors, but we just need to do some additional logic for different geo_point format parsing, which is not a huge deal. However, source is not available in sorting and filtering script contexts, so I am not sure how this is going to work there.
| @@ -417,7 +418,8 @@ public static TemporalAmount parseInterval(Source source, String value, DataType | |||
|
|
|||
| entries.add(new Entry(IntervalDayTime.class, IntervalDayTime.NAME, IntervalDayTime::new)); | |||
| entries.add(new Entry(IntervalYearMonth.class, IntervalYearMonth.NAME, IntervalYearMonth::new)); | |||
| entries.add(new Entry(GeoShape.class, GeoShape.NAME, GeoShape::new)); | |||
There was a problem hiding this comment.
Since the new literal is a non interval, the getNamedWriteables could be extracted to a separate class Literals.
There was a problem hiding this comment.
Makes sense. I think I will extract this in a separate PR that will go into master and 7.x first. And then resolve conflicts in my branch before merging it. Otherwise, I will be fighting this refactoring until I merge geosql.
|
|
||
|
|
||
| selectCitiesByDistance | ||
| SELECT region, city, ST_Distance(location, ST_WktToSQL('POINT (-71 42)')) distance FROM geo WHERE distance < 5000000 ORDER BY region, city; |
There was a problem hiding this comment.
Does GROUP BY distance work? Maybe round the result of the ST_Distance method and then use it a GROUP BY?
| throw new SqlIllegalArgumentException("distance calculation is only supported for points; received [{}]", shape1); | ||
| } | ||
| if (shape2.shapeBuilder instanceof PointBuilder == false) { | ||
| throw new SqlIllegalArgumentException("distance calculation is only supported for points; received [{}]", shape1); |
There was a problem hiding this comment.
The error message here I think it should refer to shape2.
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; |
There was a problem hiding this comment.
Can you use curly brackets for all these conditions, please? I think this is the overall style used in the other classes from SQL.
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
Since other classes besides intervals can be serialized as part of the Cursor, the getNamedWritables method should be moved from Intervals to a more generic class Literals. Relates to elastic#39973
Since other classes besides intervals can be serialized as part of the Cursor, the getNamedWritables method should be moved from Intervals to a more generic class Literals. Relates to #39973
Since other classes besides intervals can be serialized as part of the Cursor, the getNamedWritables method should be moved from Intervals to a more generic class Literals. Relates to #39973
| public StDistanceProcessor(Processor source1, Processor source2) { | ||
| super(source1, source2); | ||
| } | ||
|
|
| public String getWriteableName() { | ||
| return NAME; | ||
| } | ||
|
|
Adds ST_Distance function that works on points. No other shapes
are supported at the moments. No optimization and conversion into
geo_distance query is done yet. It will be part of another PR.
Relates to #29872