QL: Remove DateTimeFunction.dateTimeFormat() and turn eager query t…

…ranslations lazy Removes the `DateTimeFunction.dateTimeFormat()` method that was called, but had no effect on the translated queries. This change also adds laziness, so translations don't execute unnecessarily. Before the `ExpressionTranslators` did some unnecessary `Expression` -> `Query` translations, where the result queries were thrown away by later translations (by the `wrapFunctionQuery`). One of the examples, when this happens, is when one side of the `BinaryComparison` is a function. The `BinaryComparison` at the end turns into a `ScriptQuery`, but in the intermediate steps the `ExpressionTranslators` try to translate it to a `RangeQuery`. Follows elastic#68783
palesz · Feb 9, 2021 · ebaae95 · ebaae95
1 parent f1d8ded
commit ebaae95
Show file tree

Hide file tree

Showing 16 changed files with 62 additions and 109 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
@@ -84,7 +84,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) {
@@ -163,7 +163,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
@@ -54,7 +54,6 @@
 import org.elasticsearch.xpack.ql.type.DataTypes;
 import org.elasticsearch.xpack.ql.util.Check;
 import org.elasticsearch.xpack.ql.util.CollectionUtils;
-import org.elasticsearch.xpack.ql.util.Holder;
 
 import java.time.OffsetTime;
 import java.time.ZoneId;
@@ -217,15 +216,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 +238,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 +250,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,19 +271,19 @@ 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) {
             Source source = bc.source();
             String name = handler.nameOf(bc.left());
             Object value = valueOf(bc.right());
-            String format = handler.dateFormat(bc.left());
+            String format = null;
             boolean isDateLiteralComparison = false;
 
             // for a date constant comparison, we need to use a format for the date, to make sure that the format is the same
             // no matter the timezone provided by the user
-            if ((value instanceof ZonedDateTime || value instanceof OffsetTime) && format == null) {
+            if (value instanceof ZonedDateTime || value instanceof OffsetTime) {
                 DateFormatter formatter;
                 if (value instanceof ZonedDateTime) {
                     formatter = DateFormatter.forPattern(DATE_FORMAT);
@@ -341,41 +346,38 @@ 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;
-            Holder<Object> lower = new Holder<>(valueOf(r.lower()));
-            Holder<Object> upper = new Holder<>(valueOf(r.upper()));
-            Holder<String> format = new Holder<>(handler.dateFormat(val));
+        private static RangeQuery translate(Range r, TranslatorHandler handler) {
+            Object lower = valueOf(r.lower());
+            Object upper = valueOf(r.upper());
+            String format = null;
 
             // for a date constant comparison, we need to use a format for the date, to make sure that the format is the same
             // no matter the timezone provided by the user
-            if (format.get() == null) {
-                DateFormatter formatter = null;
-                if (lower.get() instanceof ZonedDateTime || upper.get() instanceof ZonedDateTime) {
-                    formatter = DateFormatter.forPattern(DATE_FORMAT);
-                } else if (lower.get() instanceof OffsetTime || upper.get() instanceof OffsetTime) {
-                    formatter = DateFormatter.forPattern(TIME_FORMAT);
+            DateFormatter formatter = null;
+            if (lower instanceof ZonedDateTime || upper instanceof ZonedDateTime) {
+                formatter = DateFormatter.forPattern(DATE_FORMAT);
+            } else if (lower instanceof OffsetTime || upper instanceof OffsetTime) {
+                formatter = DateFormatter.forPattern(TIME_FORMAT);
+            }
+            if (formatter != null) {
+                // RangeQueryBuilder accepts an Object as its parameter, but it will call .toString() on the ZonedDateTime
+                // instance which can have a slightly different format depending on the ZoneId used to create the ZonedDateTime
+                // Since RangeQueryBuilder can handle date as String as well, we'll format it as String and provide the format.
+                if (lower instanceof ZonedDateTime || lower instanceof OffsetTime) {
+                    lower = formatter.format((TemporalAccessor) lower);
                 }
-                if (formatter != null) {
-                    // RangeQueryBuilder accepts an Object as its parameter, but it will call .toString() on the ZonedDateTime
-                    // instance which can have a slightly different format depending on the ZoneId used to create the ZonedDateTime
-                    // Since RangeQueryBuilder can handle date as String as well, we'll format it as String and provide the format.
-                    if (lower.get() instanceof ZonedDateTime || lower.get() instanceof OffsetTime) {
-                        lower.set(formatter.format((TemporalAccessor) lower.get()));
-                    }
-                    if (upper.get() instanceof ZonedDateTime || upper.get() instanceof OffsetTime) {
-                        upper.set(formatter.format((TemporalAccessor) upper.get()));
-                    }
-                    format.set(formatter.pattern());
+                if (upper instanceof ZonedDateTime || upper instanceof OffsetTime) {
+                    upper = formatter.format((TemporalAccessor) upper);
                 }
+                format = formatter.pattern();
             }
-
-            query = handler.wrapFunctionQuery(r, val, new RangeQuery(r.source(), handler.nameOf(val), lower.get(), r.includeLower(),
-                                                                     upper.get(), r.includeUpper(), format.get(), r.zoneId()));
-
-            return query;
+            return new RangeQuery(
+                r.source(), handler.nameOf(r.value()), lower, r.includeLower(), upper, r.includeUpper(), format, 
+                r.zoneId());
         }
     }
 
@@ -387,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)
@@ -410,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 {
@@ -420,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;
         }
     }
 
@@ -436,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
@@ -16,6 +16,8 @@
 import org.elasticsearch.xpack.ql.type.DataType;
 import org.elasticsearch.xpack.ql.type.DataTypeConverter;
 
+import java.util.function.Supplier;
+
 public class QlTranslatorHandler implements TranslatorHandler {
 
     @Override
@@ -24,9 +26,9 @@ public Query asQuery(Expression e) {
     }
 
     @Override
-    public Query wrapFunctionQuery(ScalarFunction sf, Expression field, Query q) {
+    public Query wrapFunctionQuery(ScalarFunction sf, Expression field, Supplier<Query> querySupplier) {
         if (field instanceof FieldAttribute) {
-            return ExpressionTranslator.wrapIfNested(q, field);
+            return ExpressionTranslator.wrapIfNested(querySupplier.get(), field);
         }
         return new ScriptQuery(sf.source(), sf.asScript());
     }
@@ -40,11 +42,6 @@ public String nameOf(Expression e) {
         }
     }
 
-    @Override
-    public String dateFormat(Expression e) {
-        return null;
-    }
-
     @Override
     public Object convert(Object value, DataType dataType) {
         return DataTypeConverter.convert(value, dataType);

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
@@ -12,6 +12,8 @@
 import org.elasticsearch.xpack.ql.querydsl.query.Query;
 import org.elasticsearch.xpack.ql.type.DataType;
 
+import java.util.function.Supplier;
+
 /**
  * Parameterized handler used during query translation.
  *
@@ -21,11 +23,9 @@ public interface TranslatorHandler {
 
     Query asQuery(Expression e);
 
-    Query wrapFunctionQuery(ScalarFunction sf, Expression field, Query q);
+    Query wrapFunctionQuery(ScalarFunction sf, Expression field, Supplier<Query> querySupplier);
 
     String nameOf(Expression e);
 
-    String dateFormat(Expression e);
-
     Object convert(Object value, DataType dataType);
 }
diff --git a/...ava/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFunction.java b/...ava/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFunction.java
@@ -63,9 +63,6 @@ public DataType dataType() {
         return DataTypes.INTEGER;
     }
 
-    // used for applying ranges
-    public abstract String dateTimeFormat();
-
     protected DateTimeExtractor extractor() {
         return extractor;
     }

diff --git a/...main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DayOfMonth.java b/...main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DayOfMonth.java
@@ -31,8 +31,4 @@ protected DayOfMonth replaceChild(Expression newChild) {
         return new DayOfMonth(source(), newChild, zoneId());
     }
 
-    @Override
-    public String dateTimeFormat() {
-        return "d";
-    }
 }
diff --git a/.../main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DayOfYear.java b/.../main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DayOfYear.java
@@ -32,8 +32,4 @@ protected UnaryScalarFunction replaceChild(Expression newChild) {
         return new DayOfYear(source(), newChild, zoneId());
     }
 
-    @Override
-    public String dateTimeFormat() {
-        return "D";
-    }
 }
diff --git a/.../main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/HourOfDay.java b/.../main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/HourOfDay.java
@@ -31,8 +31,4 @@ protected HourOfDay replaceChild(Expression newChild) {
         return new HourOfDay(source(), newChild, zoneId());
     }
 
-    @Override
-    public String dateTimeFormat() {
-        return "hour";
-    }
 }
diff --git a/...in/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/IsoDayOfWeek.java b/...in/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/IsoDayOfWeek.java
@@ -31,8 +31,4 @@ protected IsoDayOfWeek replaceChild(Expression newChild) {
         return new IsoDayOfWeek(source(), newChild, zoneId());
     }
 
-    @Override
-    public String dateTimeFormat() {
-        return "e";
-    }
 }
diff --git a/...n/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/IsoWeekOfYear.java b/...n/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/IsoWeekOfYear.java
@@ -31,8 +31,4 @@ protected IsoWeekOfYear replaceChild(Expression newChild) {
         return new IsoWeekOfYear(source(), newChild, zoneId());
     }
 
-    @Override
-    public String dateTimeFormat() {
-        return "w";
-    }
 }
diff --git a/...ain/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/MinuteOfDay.java b/...ain/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/MinuteOfDay.java
@@ -32,8 +32,4 @@ protected MinuteOfDay replaceChild(Expression newChild) {
         return new MinuteOfDay(source(), newChild, zoneId());
     }
 
-    @Override
-    public String dateTimeFormat() {
-        throw new UnsupportedOperationException("is there a format for it?");
-    }
 }
diff --git a/...in/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/MinuteOfHour.java b/...in/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/MinuteOfHour.java
@@ -31,8 +31,4 @@ protected MinuteOfHour replaceChild(Expression newChild) {
         return new MinuteOfHour(source(), newChild, zoneId());
     }
 
-    @Override
-    public String dateTimeFormat() {
-        return "m";
-    }
 }
diff --git a/...ain/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/MonthOfYear.java b/...ain/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/MonthOfYear.java
@@ -31,8 +31,4 @@ protected MonthOfYear replaceChild(Expression newChild) {
         return new MonthOfYear(source(), newChild, zoneId());
     }
 
-    @Override
-    public String dateTimeFormat() {
-        return "M";
-    }
 }
diff --git a/.../java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/SecondOfMinute.java b/.../java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/SecondOfMinute.java
@@ -31,8 +31,4 @@ protected SecondOfMinute replaceChild(Expression newChild) {
         return new SecondOfMinute(source(), newChild, zoneId());
     }
 
-    @Override
-    public String dateTimeFormat() {
-        return "s";
-    }
 }
diff --git a/...l/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/Year.java b/...l/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/Year.java
@@ -33,11 +33,6 @@ protected Year replaceChild(Expression newChild) {
         return new Year(source(), newChild, zoneId());
     }
 
-    @Override
-    public String dateTimeFormat() {
-        return "year";
-    }
-
     @Override
     public String calendarInterval() {
         return Histogram.YEAR_INTERVAL;