Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QL: Remove the unnecessary DateTimeFunction.dateTimeFormat() #68788

Merged
merged 3 commits into from
Feb 17, 2021
Merged

QL: Remove the unnecessary DateTimeFunction.dateTimeFormat() #68788

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ebaae95
QL: Remove `DateTimeFunction.dateTimeFormat()` and turn eager query t…
Feb 9, 2021
96cf97b
Merge remote-tracking branch 'origin/master'
Feb 10, 2021
94b6be1
Reverting the changes related to lazyness
Feb 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/QueryTranslator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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()));
}
}

Expand Down
87 changes: 47 additions & 40 deletions x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}

Expand All @@ -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) {
Expand All @@ -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;
}
}

Expand All @@ -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);
Expand Down Expand Up @@ -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());
}
}

Expand All @@ -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)
Expand All @@ -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 {
Expand All @@ -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;
}
}

Expand All @@ -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) {
Expand Down
11 changes: 4 additions & 7 deletions x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/QlTranslatorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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());
}
Expand All @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/TranslatorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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);
}
3 changes: 0 additions & 3 deletions ...ava/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,6 @@ public DataType dataType() {
return DataTypes.INTEGER;
}

// used for applying ranges
public abstract String dateTimeFormat();

protected DateTimeExtractor extractor() {
return extractor;
}
Expand Down
4 changes: 0 additions & 4 deletions ...main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DayOfMonth.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,4 @@ protected DayOfMonth replaceChild(Expression newChild) {
return new DayOfMonth(source(), newChild, zoneId());
}

@Override
public String dateTimeFormat() {
return "d";
}
}
4 changes: 0 additions & 4 deletions .../main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DayOfYear.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,4 @@ protected UnaryScalarFunction replaceChild(Expression newChild) {
return new DayOfYear(source(), newChild, zoneId());
}

@Override
public String dateTimeFormat() {
return "D";
}
}
4 changes: 0 additions & 4 deletions .../main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/HourOfDay.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,4 @@ protected HourOfDay replaceChild(Expression newChild) {
return new HourOfDay(source(), newChild, zoneId());
}

@Override
public String dateTimeFormat() {
return "hour";
}
}
4 changes: 0 additions & 4 deletions ...in/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/IsoDayOfWeek.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,4 @@ protected IsoDayOfWeek replaceChild(Expression newChild) {
return new IsoDayOfWeek(source(), newChild, zoneId());
}

@Override
public String dateTimeFormat() {
return "e";
}
}
4 changes: 0 additions & 4 deletions ...n/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/IsoWeekOfYear.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,4 @@ protected IsoWeekOfYear replaceChild(Expression newChild) {
return new IsoWeekOfYear(source(), newChild, zoneId());
}

@Override
public String dateTimeFormat() {
return "w";
}
}
4 changes: 0 additions & 4 deletions ...ain/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/MinuteOfDay.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?");
}
}
4 changes: 0 additions & 4 deletions ...in/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/MinuteOfHour.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,4 @@ protected MinuteOfHour replaceChild(Expression newChild) {
return new MinuteOfHour(source(), newChild, zoneId());
}

@Override
public String dateTimeFormat() {
return "m";
}
}
4 changes: 0 additions & 4 deletions ...ain/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/MonthOfYear.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,4 @@ protected MonthOfYear replaceChild(Expression newChild) {
return new MonthOfYear(source(), newChild, zoneId());
}

@Override
public String dateTimeFormat() {
return "M";
}
}
4 changes: 0 additions & 4 deletions .../java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/SecondOfMinute.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,4 @@ protected SecondOfMinute replaceChild(Expression newChild) {
return new SecondOfMinute(source(), newChild, zoneId());
}

@Override
public String dateTimeFormat() {
return "s";
}
}
5 changes: 0 additions & 5 deletions ...l/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/Year.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading