QL: Remove the unnecessary DateTimeFunction.dateTimeFormat()

The `DateTimeFunction.dateTimeFormat()` method that was called, but had no effect on the translated queries. 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 10, 2021 · 6ef3e13 · 6ef3e13
1 parent 683368c
commit 6ef3e13
Show file tree

Hide file tree

Showing 15 changed files with 23 additions and 87 deletions.
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;
@@ -273,12 +272,12 @@ 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);
@@ -345,36 +344,33 @@ public static Query doTranslate(Range r, TranslatorHandler handler) {
             Expression val = r.value();
 
             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));
+            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()));
-
+            query = handler.wrapFunctionQuery(r, val,
+                    new RangeQuery(r.source(), handler.nameOf(val), lower, r.includeLower(), upper, r.includeUpper(), format, r.zoneId()));
+
             return query;
         }
     }

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
@@ -40,11 +40,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
@@ -25,7 +25,5 @@ public interface TranslatorHandler {
 
     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() {
-        return null;
-    }
 }
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;

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
@@ -16,7 +16,6 @@
 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.datetime.DateTimeFunction;
 import org.elasticsearch.xpack.sql.expression.function.scalar.geo.StDistance;
 import org.elasticsearch.xpack.sql.type.SqlDataTypeConverter;
 
@@ -49,14 +48,6 @@ public String nameOf(Expression e) {
         return QueryTranslator.nameOf(e);
     }
 
-    @Override
-    public String dateFormat(Expression e) {
-        if (e instanceof DateTimeFunction) {
-            return ((DateTimeFunction) e).dateTimeFormat();
-        }
-        return null;
-    }
-
     @Override
     public Object convert(Object value, DataType dataType) {
         return SqlDataTypeConverter.convert(value, dataType);