QL: Turn eager query translations lazy (elastic#69205)

Before the `ExpressionTranslators` did some unnecessary `Expression` -> `Query` translations, where the result queries were thrown away by later translations (by the `wrapFunctionQuery`). This change adds laziness, so translations don't execute unnecessarily. Follows elastic#68783 and elastic#68788
palesz · Mar 2, 2021 · 4a0b661 · 4a0b661
1 parent 7a9e527
commit 4a0b661
Show file tree

Hide file tree

Showing 10 changed files with 59 additions and 49 deletions.
diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/QueryTranslator.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/QueryTranslator.java
@@ -85,7 +85,7 @@ protected Query asQuery(InsensitiveBinaryComparison bc, TranslatorHandler handle
 
         public static Query doTranslate(InsensitiveBinaryComparison bc, TranslatorHandler handler) {
             checkInsensitiveComparison(bc);
-            return handler.wrapFunctionQuery(bc, bc.left(), translate(bc, handler));
+            return handler.wrapFunctionQuery(bc, bc.left(), () -> translate(bc, handler));
         }
 
         public static void checkInsensitiveComparison(InsensitiveBinaryComparison bc) {
@@ -164,7 +164,7 @@ public static Query doTranslate(ScalarFunction f, TranslatorHandler handler) {
                 }
             }
 
-            return handler.wrapFunctionQuery(f, f, new ScriptQuery(f.source(), f.asScript()));
+            return handler.wrapFunctionQuery(f, f, () -> new ScriptQuery(f.source(), f.asScript()));
         }
     }
 

diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java
@@ -217,15 +217,17 @@ protected Query asQuery(IsNotNull isNotNull, TranslatorHandler handler) {
         }
 
         public static Query doTranslate(IsNotNull isNotNull, TranslatorHandler handler) {
-            Query query = null;
+            return handler.wrapFunctionQuery(isNotNull, isNotNull.field(), () -> translate(isNotNull, handler));
+        }
 
+        private static Query translate(IsNotNull isNotNull, TranslatorHandler handler) {
+            Query query = null;
             if (isNotNull.field() instanceof FieldAttribute) {
                 query = new ExistsQuery(isNotNull.source(), handler.nameOf(isNotNull.field()));
             } else {
                 query = new ScriptQuery(isNotNull.source(), isNotNull.asScript());
             }
-
-            return handler.wrapFunctionQuery(isNotNull, isNotNull.field(), query);
+            return query;
         }
     }
 
@@ -237,6 +239,10 @@ protected Query asQuery(IsNull isNull, TranslatorHandler handler) {
         }
 
         public static Query doTranslate(IsNull isNull, TranslatorHandler handler) {
+            return handler.wrapFunctionQuery(isNull, isNull.field(), () -> translate(isNull, handler));
+        }
+
+        private static Query translate(IsNull isNull, TranslatorHandler handler) {
             Query query = null;
 
             if (isNull.field() instanceof FieldAttribute) {
@@ -245,7 +251,7 @@ public static Query doTranslate(IsNull isNull, TranslatorHandler handler) {
                 query = new ScriptQuery(isNull.source(), isNull.asScript());
             }
 
-            return handler.wrapFunctionQuery(isNull, isNull.field(), query);
+            return query;
         }
     }
 
@@ -266,7 +272,7 @@ public static void checkBinaryComparison(BinaryComparison bc) {
 
         public static Query doTranslate(BinaryComparison bc, TranslatorHandler handler) {
             checkBinaryComparison(bc);
-            return handler.wrapFunctionQuery(bc, bc.left(), translate(bc, handler));
+            return handler.wrapFunctionQuery(bc, bc.left(), () -> translate(bc, handler));
         }
 
         private static Query translate(BinaryComparison bc, TranslatorHandler handler) {
@@ -341,10 +347,11 @@ protected Query asQuery(Range r, TranslatorHandler handler) {
             return doTranslate(r, handler);
         }
 
-        public static Query doTranslate(Range r, TranslatorHandler handler) {
-            Expression val = r.value();
+        public static Query doTranslate(Range r, TranslatorHandler handler) {            
+            return handler.wrapFunctionQuery(r, r.value(), () -> translate(r, handler));
+        }
 
-            Query query = null;
+        private static RangeQuery translate(Range r, TranslatorHandler handler) {            
             Object lower = valueOf(r.lower());
             Object upper = valueOf(r.upper());
             String format = null;
@@ -369,10 +376,8 @@ public static Query doTranslate(Range r, TranslatorHandler handler) {
                 }
                 format = formatter.pattern();
             }
-            query = handler.wrapFunctionQuery(r, val,
-                    new RangeQuery(r.source(), handler.nameOf(val), lower, r.includeLower(), upper, r.includeUpper(), format, r.zoneId()));
-
-            return query;
+            return new RangeQuery(
+                r.source(), handler.nameOf(r.value()), lower, r.includeLower(), upper, r.includeUpper(), format, r.zoneId());
         }
     }
 
@@ -384,6 +389,10 @@ protected Query asQuery(In in, TranslatorHandler handler) {
         }
 
         public static Query doTranslate(In in, TranslatorHandler handler) {
+            return handler.wrapFunctionQuery(in, in.value(), () -> translate(in, handler));
+        }
+
+        private static Query translate(In in, TranslatorHandler handler) {
             Query q;
             if (in.value() instanceof FieldAttribute) {
                 // equality should always be against an exact match (which is important for strings)
@@ -407,8 +416,9 @@ public static Query doTranslate(In in, TranslatorHandler handler) {
                         assert o instanceof ZonedDateTime : "expected a ZonedDateTime, but got: " + o.getClass().getName();
                         // see comment in Ranges#doTranslate() as to why formatting as String is required
                         String zdt = formatter.format((ZonedDateTime) o);
-                        RangeQuery right = new RangeQuery(in.source(), fa.exactAttribute().name(),
-                                zdt, true, zdt, true, formatter.pattern(), in.zoneId());
+                        RangeQuery right = new RangeQuery(
+                            in.source(), fa.exactAttribute().name(),
+                            zdt, true, zdt, true, formatter.pattern(), in.zoneId());
                         q = q == null ? right : new BoolQuery(in.source(), false, q, right);
                     }
                 } else {
@@ -417,7 +427,7 @@ public static Query doTranslate(In in, TranslatorHandler handler) {
             } else {
                 q = new ScriptQuery(in.source(), in.asScript());
             }
-            return handler.wrapFunctionQuery(in, in.value(), q);
+            return q;
         }
     }
 
@@ -433,7 +443,7 @@ public static Query doTranslate(ScalarFunction f, TranslatorHandler handler) {
             if (q != null) {
                 return q;
             }
-            return handler.wrapFunctionQuery(f, f, new ScriptQuery(f.source(), f.asScript()));
+            return handler.wrapFunctionQuery(f, f, () -> new ScriptQuery(f.source(), f.asScript()));
         }
 
         public static Query doKnownTranslate(ScalarFunction f, TranslatorHandler handler) {

diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/QlTranslatorHandler.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/QlTranslatorHandler.java
@@ -8,11 +8,8 @@
 package org.elasticsearch.xpack.ql.planner;
 
 import org.elasticsearch.xpack.ql.expression.Expression;
-import org.elasticsearch.xpack.ql.expression.FieldAttribute;
 import org.elasticsearch.xpack.ql.expression.NamedExpression;
-import org.elasticsearch.xpack.ql.expression.function.scalar.ScalarFunction;
 import org.elasticsearch.xpack.ql.querydsl.query.Query;
-import org.elasticsearch.xpack.ql.querydsl.query.ScriptQuery;
 import org.elasticsearch.xpack.ql.type.DataType;
 import org.elasticsearch.xpack.ql.type.DataTypeConverter;
 
@@ -23,14 +20,6 @@ public Query asQuery(Expression e) {
         return ExpressionTranslators.toQuery(e, this);
     }
 
-    @Override
-    public Query wrapFunctionQuery(ScalarFunction sf, Expression field, Query q) {
-        if (field instanceof FieldAttribute) {
-            return ExpressionTranslator.wrapIfNested(q, field);
-        }
-        return new ScriptQuery(sf.source(), sf.asScript());
-    }
-
     @Override
     public String nameOf(Expression e) {
         if (e instanceof NamedExpression) {

diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/TranslatorHandler.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/TranslatorHandler.java
@@ -8,10 +8,14 @@
 package org.elasticsearch.xpack.ql.planner;
 
 import org.elasticsearch.xpack.ql.expression.Expression;
+import org.elasticsearch.xpack.ql.expression.FieldAttribute;
 import org.elasticsearch.xpack.ql.expression.function.scalar.ScalarFunction;
 import org.elasticsearch.xpack.ql.querydsl.query.Query;
+import org.elasticsearch.xpack.ql.querydsl.query.ScriptQuery;
 import org.elasticsearch.xpack.ql.type.DataType;
 
+import java.util.function.Supplier;
+
 /**
  * Parameterized handler used during query translation.
  *
@@ -21,7 +25,12 @@ public interface TranslatorHandler {
 
     Query asQuery(Expression e);
 
-    Query wrapFunctionQuery(ScalarFunction sf, Expression field, Query q);
+    default Query wrapFunctionQuery(ScalarFunction sf, Expression field, Supplier<Query> querySupplier) {
+        if (field instanceof FieldAttribute) {
+            return ExpressionTranslator.wrapIfNested(querySupplier.get(), field);
+        }
+        return new ScriptQuery(sf.source(), sf.asScript());
+    }
 
     String nameOf(Expression e);
 

diff --git a/x-pack/plugin/ql/src/test/resources/mapping-multi-field-variation.json b/x-pack/plugin/ql/src/test/resources/mapping-multi-field-variation.json
@@ -48,6 +48,12 @@
         },
         "foo_type" : { "type" : "foo" },
         "point": {"type" : "geo_point"},
-        "shape": {"type" : "geo_shape"}
+        "shape": {"type" : "geo_shape"},
+        "nested": {
+            "type": "nested",
+            "properties": {
+                "point": {"type" : "geo_point"}
+            }
+        }
     }
 }
diff --git a/x-pack/plugin/ql/src/test/resources/mapping-multi-field-with-nested.json b/x-pack/plugin/ql/src/test/resources/mapping-multi-field-with-nested.json
@@ -88,6 +88,9 @@
                 },
                 "start_date" : {
                     "type" : "date"
+                },
+                "location" : {
+                    "type" : "geo_point"
                 }
             }
         }

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java
@@ -30,6 +30,7 @@
 import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.LessThan;
 import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.LessThanOrEqual;
 import org.elasticsearch.xpack.ql.expression.predicate.regex.RegexMatch;
+import org.elasticsearch.xpack.ql.planner.ExpressionTranslator;
 import org.elasticsearch.xpack.ql.planner.ExpressionTranslators;
 import org.elasticsearch.xpack.ql.planner.TranslatorHandler;
 import org.elasticsearch.xpack.ql.querydsl.query.GeoDistanceQuery;
@@ -407,8 +408,9 @@ private static Query translateQuery(BinaryComparison bc, TranslatorHandler handl
                             Geometry geometry = ((GeoShape) geoShape).toGeometry();
                             if (geometry instanceof Point) {
                                 String field = nameOf(stDistance.left());
-                                return new GeoDistanceQuery(source, field, ((Number) value).doubleValue(),
+                                Query query = new GeoDistanceQuery(source, field, ((Number) value).doubleValue(),
                                     ((Point) geometry).getY(), ((Point) geometry).getX());
+                                return ExpressionTranslator.wrapIfNested(query, stDistance.left());
                             }
                         }
                     }

diff --git a/...ck/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/SqlTranslatorHandler.java b/...ck/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/SqlTranslatorHandler.java
@@ -8,15 +8,9 @@
 package org.elasticsearch.xpack.sql.planner;
 
 import org.elasticsearch.xpack.ql.expression.Expression;
-import org.elasticsearch.xpack.ql.expression.FieldAttribute;
-import org.elasticsearch.xpack.ql.expression.function.scalar.ScalarFunction;
-import org.elasticsearch.xpack.ql.planner.ExpressionTranslator;
 import org.elasticsearch.xpack.ql.planner.TranslatorHandler;
-import org.elasticsearch.xpack.ql.querydsl.query.GeoDistanceQuery;
 import org.elasticsearch.xpack.ql.querydsl.query.Query;
-import org.elasticsearch.xpack.ql.querydsl.query.ScriptQuery;
 import org.elasticsearch.xpack.ql.type.DataType;
-import org.elasticsearch.xpack.sql.expression.function.scalar.geo.StDistance;
 import org.elasticsearch.xpack.sql.type.SqlDataTypeConverter;
 
 public class SqlTranslatorHandler implements TranslatorHandler {
@@ -32,17 +26,6 @@ public Query asQuery(Expression e) {
         return QueryTranslator.toQuery(e, onAggs).query;
     }
 
-    @Override
-    public Query wrapFunctionQuery(ScalarFunction sf, Expression field, Query q) {
-        if (field instanceof StDistance && q instanceof GeoDistanceQuery) {
-            return ExpressionTranslator.wrapIfNested(q, ((StDistance) field).left());
-        }
-        if (field instanceof FieldAttribute) {
-            return ExpressionTranslator.wrapIfNested(q, field);
-        }
-        return new ScriptQuery(sf.source(), sf.asScript());
-    }
-
     @Override
     public String nameOf(Expression e) {
         return QueryTranslator.nameOf(e);

diff --git a/...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java
@@ -567,6 +567,10 @@ public void testWhereOnNested() {
         accept("SELECT int FROM test WHERE dep.start_date > '2020-01-30'::date AND (int > 10 OR dep.end_date IS NULL)");
         accept("SELECT int FROM test WHERE dep.start_date > '2020-01-30'::date AND (int > 10 OR dep.end_date IS NULL) " +
                "OR NOT(dep.start_date >= '2020-01-01')");
+        String operator = randomFrom("<", "<=");
+        assertEquals("1:42: WHERE isn't (yet) compatible with scalar functions on nested fields [dep.location]",
+            error("SELECT shape FROM test " +
+                "WHERE ST_Distance(dep.location, ST_WKTToSQL('point (10 20)')) " + operator + " 25"));
     }
 
     public void testOrderByOnNested() {

diff --git a/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java
@@ -1309,6 +1309,10 @@ public void testTranslateStDistanceToQuery() {
         assertEquals(20.0, gq.lat(), 0.00001);
         assertEquals(10.0, gq.lon(), 0.00001);
         assertEquals(25.0, gq.distance(), 0.00001);
+        EsQueryExec eqe = (EsQueryExec) optimizeAndPlan(p);
+        assertThat(eqe.queryContainer().toString().replaceAll("\\s+", ""),
+            containsString("{\"geo_distance\":{\"point\":[10.0,20.0],\"distance\":25.0," +
+                "\"distance_type\":\"arc\",\"validation_method\":\"STRICT\","));
     }
 
     public void testTranslateStXY() {