SQL: Convert ST_Distance into query when possible #40595
Conversation
Adds additional optimization logic to convert ST_Distance function calls into geo_distance query when it is called in WHERE clauses.
imotov
added
>enhancement
:Analytics/Geo
|
Pinging @elastic/es-search |
|
Pinging @elastic/es-analytics-geo |
| @@ -134,6 +135,8 @@ public LogicalPlan optimize(LogicalPlan verified) { | |||
| // needs to occur before BinaryComparison combinations (see class) | |||
| new PropagateEquals(), | |||
| new CombineBinaryComparisons(), | |||
| // Geo | |||
There was a problem hiding this comment.
There's already a rule that does this for BinaryOperators - if StDistance extends that calls (and it looks like it can), the swapping will happen automatically.
| @@ -675,6 +680,21 @@ private static Query translateQuery(BinaryComparison bc) { | |||
| return new RangeQuery(source, name, value, true, null, false, format); | |||
| } | |||
| if (bc instanceof LessThan) { | |||
| if (bc.left() instanceof StDistance && value instanceof Number) { | |||
There was a problem hiding this comment.
Do we expect more similar functions to be optimized in a similar fashion? If the answer is yes, then likely we could have more infrastructure to help identify such patterns.
There was a problem hiding this comment.
I don't think we will. There will be some cases for shapes, but I don't see the similarity clearly at the moment to create a good abstraction at the moment. I would rather postpone this until we get there and then try to wrap it.
| @@ -675,6 +680,21 @@ private static Query translateQuery(BinaryComparison bc) { | |||
| return new RangeQuery(source, name, value, true, null, false, format); | |||
| } | |||
| if (bc instanceof LessThan) { | |||
There was a problem hiding this comment.
Shouldn't a similar query be applied for LessThanOrEqual?
There was a problem hiding this comment.
That's a good question. Now that I am thinking about it and after discussions with @iverase, it looks like it should be applied only to LessThanOrEqual, but since the calculation is not precise, maybe we can get away with implementing it for both.
| @@ -83,4 +72,22 @@ protected Pipe makePipe() { | |||
| protected String scriptMethodName() { | |||
| return "stDistance"; | |||
| } | |||
|
|
|||
| public static class StDistanceFunction implements PredicateBiFunction<Object, Object, Double> { | |||
There was a problem hiding this comment.
I would define it in as a normal class in a separate file instead.
| import static org.elasticsearch.xpack.sql.expression.gen.script.ParamsBuilder.paramsBuilder; | ||
|
|
||
| /** | ||
| * Calculates the distance between two points | ||
| */ | ||
| public class StDistance extends BinaryScalarFunction { | ||
| public class StDistance extends BinaryOperator<Object, Object, Double, StDistance.StDistanceFunction> { |
There was a problem hiding this comment.
I'd rename it to StDistanceOperator
There was a problem hiding this comment.
ST_DISTANCE(sh1, sh2) is not an operator, we are just reusing some logic that can be applied to both.
There was a problem hiding this comment.
Sorry, I had a mistake. My suggestion is to rename StDistance to StDistanceOperation and StDistanceFunction to StDistance similarly to other functions/operators in SQL.
Adds additional optimization logic to convert ST_Distance function
calls into geo_distance query when it is called in WHERE clauses.