Skip to content

Commit

Permalink
SQL: Fix disjunctions (and IN) with multiple date math expressions (#…
Browse files Browse the repository at this point in the history 
…76424) (#77078)

* SQL: Fix disjunctions with multiple date math expressions

* review comments

* remove redundant FieldAttribute parameter

* address comments and add more specs

* fix typo
# Conflicts:
#	x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java
#	x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java
  • Loading branch information
Lukas Wegmann committed Aug 31, 2021
1 parent d0c4ec9 commit 8fdaae5
Show file tree
Hide file tree
Showing 10 changed files with 259 additions and 96 deletions.
14 changes: 14 additions & 0 deletions docs/reference/sql/functions/date-time.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,20 @@ s|Description
| `INTERVAL '45:01.23' MINUTES TO SECONDS` | 45 minutes, 1 second and 230000000 nanoseconds
|===

==== Comparison

Date/time fields can be compared to <<date-math, date math>> expressions with the equality (`=`) and `IN` operators:

[source, sql]
--------------------------------------------------
include-tagged::{sql-specs}/docs/docs.csv-spec[dtDateMathEquals]
--------------------------------------------------

[source, sql]
--------------------------------------------------
include-tagged::{sql-specs}/docs/docs.csv-spec[dtDateMathIn]
--------------------------------------------------

==== Operators

Basic arithmetic operators (`+`, `-`, `*`) support date/time parameters as indicated below:
Expand Down
13 changes: 5 additions & 8 deletions x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/QueryTranslator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,25 +113,22 @@ public static void checkInsensitiveComparison(InsensitiveBinaryComparison bc) {
}

private static Query translate(InsensitiveBinaryComparison bc, TranslatorHandler handler) {
FieldAttribute field = checkIsFieldAttribute(bc.left());
Source source = bc.source();
String name = handler.nameOf(bc.left());
Object value = valueOf(bc.right());

if (bc instanceof InsensitiveEquals || bc instanceof InsensitiveNotEquals) {
if (bc.left() instanceof FieldAttribute) {
// equality should always be against an exact match
// (which is important for strings)
name = ((FieldAttribute) bc.left()).exactAttribute().name();
}
// equality should always be against an exact match
// (which is important for strings)
String name = field.exactAttribute().name();

Query query = new TermQuery(source, name, value, true);

if (bc instanceof InsensitiveNotEquals) {
query = new NotQuery(source, query);
}

return query;
}

throw new QlIllegalArgumentException("Don't know how to translate binary comparison [{}] in [{}]", bc.right().nodeString(), bc);
}
}
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,8 @@ private Expression propagate(And and) {
} else if (ex instanceof Equals || ex instanceof NullEquals) {
BinaryComparison otherEq = (BinaryComparison) ex;
// equals on different values evaluate to FALSE
if (otherEq.right().foldable()) {
// ignore date/time fields as equality comparison might actually be a range check
if (otherEq.right().foldable() && DataTypes.isDateTime(otherEq.left().dataType()) == false) {
for (BinaryComparison eq : equals) {
if (otherEq.left().semanticEquals(eq.left())) {
Integer comp = BinaryComparison.compare(eq.right().fold(), otherEq.right().fold());
Expand Down
6 changes: 6 additions & 0 deletions x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.xpack.ql.expression.FieldAttribute;
import org.elasticsearch.xpack.ql.querydsl.query.NestedQuery;
import org.elasticsearch.xpack.ql.querydsl.query.Query;
import org.elasticsearch.xpack.ql.util.Check;
import org.elasticsearch.xpack.ql.util.ReflectionUtils;

public abstract class ExpressionTranslator<E extends Expression> {
Expand All @@ -33,4 +34,9 @@ public static Query wrapIfNested(Query query, Expression exp) {
}
return query;
}

public static FieldAttribute checkIsFieldAttribute(Expression e) {
Check.isTrue(e instanceof FieldAttribute, "Expected a FieldAttribute but received [{}]", e);
return (FieldAttribute) e;
}
}
100 changes: 40 additions & 60 deletions x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,15 @@
import org.elasticsearch.xpack.ql.querydsl.query.TermsQuery;
import org.elasticsearch.xpack.ql.querydsl.query.WildcardQuery;
import org.elasticsearch.xpack.ql.tree.Source;
import org.elasticsearch.xpack.ql.type.DataType;
import org.elasticsearch.xpack.ql.type.DataTypes;
import org.elasticsearch.xpack.ql.util.Check;
import org.elasticsearch.xpack.ql.util.CollectionUtils;

import java.time.OffsetTime;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.time.temporal.TemporalAccessor;
import java.util.Arrays;
import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -221,13 +220,7 @@ public static Query doTranslate(IsNotNull isNotNull, TranslatorHandler 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 query;
return new ExistsQuery(isNotNull.source(), handler.nameOf(isNotNull.field()));
}
}

Expand All @@ -243,15 +236,7 @@ public static Query doTranslate(IsNull isNull, TranslatorHandler handler) {
}

private static Query translate(IsNull isNull, TranslatorHandler handler) {
Query query = null;

if (isNull.field() instanceof FieldAttribute) {
query = new NotQuery(isNull.source(), new ExistsQuery(isNull.source(), handler.nameOf(isNull.field())));
} else {
query = new ScriptQuery(isNull.source(), isNull.asScript());
}

return query;
return new NotQuery(isNull.source(), new ExistsQuery(isNull.source(), handler.nameOf(isNull.field())));
}
}

Expand All @@ -275,9 +260,10 @@ public static Query doTranslate(BinaryComparison bc, TranslatorHandler handler)
return handler.wrapFunctionQuery(bc, bc.left(), () -> translate(bc, handler));
}

private static Query translate(BinaryComparison bc, TranslatorHandler handler) {
static Query translate(BinaryComparison bc, TranslatorHandler handler) {
FieldAttribute field = checkIsFieldAttribute(bc.left());
Source source = bc.source();
String name = handler.nameOf(bc.left());
String name = handler.nameOf(field);
Object value = valueOf(bc.right());
String format = null;
boolean isDateLiteralComparison = false;
Expand All @@ -301,7 +287,7 @@ private static Query translate(BinaryComparison bc, TranslatorHandler handler) {
}

ZoneId zoneId = null;
if (DataTypes.isDateTime(bc.left().dataType())) {
if (DataTypes.isDateTime(field.dataType())) {
zoneId = bc.zoneId();
}
if (bc instanceof GreaterThan) {
Expand All @@ -317,11 +303,10 @@ private static Query translate(BinaryComparison bc, TranslatorHandler handler) {
return new RangeQuery(source, name, null, false, value, true, format, zoneId);
}
if (bc instanceof Equals || bc instanceof NullEquals || bc instanceof NotEquals) {
if (bc.left() instanceof FieldAttribute) {
// equality should always be against an exact match
// (which is important for strings)
name = ((FieldAttribute) bc.left()).exactAttribute().name();
}
// equality should always be against an exact match
// (which is important for strings)
name = field.exactAttribute().name();

Query query;
if (isDateLiteralComparison) {
// dates equality uses a range query because it's the one that has a "format" parameter
Expand All @@ -347,11 +332,11 @@ protected Query asQuery(Range r, TranslatorHandler handler) {
return doTranslate(r, handler);
}

public static Query doTranslate(Range r, TranslatorHandler handler) {
public static Query doTranslate(Range r, TranslatorHandler handler) {
return handler.wrapFunctionQuery(r, r.value(), () -> translate(r, handler));
}

private static RangeQuery translate(Range r, TranslatorHandler handler) {
private static RangeQuery translate(Range r, TranslatorHandler handler) {
Object lower = valueOf(r.lower());
Object upper = valueOf(r.upper());
String format = null;
Expand Down Expand Up @@ -393,41 +378,36 @@ public static Query doTranslate(In in, TranslatorHandler 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)
FieldAttribute fa = (FieldAttribute) in.value();
DataType dt = fa.dataType();

List<Expression> list = in.list();
Set<Object> set = new LinkedHashSet<>(CollectionUtils.mapSize(list.size()));
list.forEach(e -> {
// TODO: this needs to be handled inside the optimizer
if (DataTypes.isNull(e.dataType()) == false) {
set.add(handler.convert(valueOf(e), dt));
}
});

if (DataTypes.isDateTime(dt)) {
DateFormatter formatter = DateFormatter.forPattern(DATE_FORMAT);

q = null;
for (Object o : set) {
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());
q = q == null ? right : new BoolQuery(in.source(), false, q, right);
FieldAttribute field = checkIsFieldAttribute(in.value());
boolean isDateTimeComparison = DataTypes.isDateTime(field.dataType());

Set<Object> terms = new LinkedHashSet<>();
List<Query> queries = new ArrayList<>();

for (Expression rhs : in.list()) {
if (DataTypes.isNull(rhs.dataType()) == false) {
if (isDateTimeComparison) {
// delegates to BinaryComparisons translator to ensure consistent handling of date and time values
Query query = BinaryComparisons.translate(new Equals(in.source(), in.value(), rhs, in.zoneId()), handler);

if (query instanceof TermQuery) {
terms.add(((TermQuery) query).value());
} else {
queries.add(query);
}
} else {
terms.add(valueOf(rhs));
}
} else {
q = new TermsQuery(in.source(), fa.exactAttribute().name(), set);
}
} else {
q = new ScriptQuery(in.source(), in.asScript());
}
return q;

if (terms.isEmpty() == false) {
String fieldName = field.exactAttribute().name();
queries.add(new TermsQuery(in.source(), fieldName, terms));
}

return queries.stream()
.reduce((q1, q2) -> or(in.source(), q1, q2)).get();
}
}

Expand Down
13 changes: 13 additions & 0 deletions x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.elasticsearch.xpack.ql.tree.NodeInfo;
import org.elasticsearch.xpack.ql.tree.Source;
import org.elasticsearch.xpack.ql.type.DataType;
import org.elasticsearch.xpack.ql.type.DataTypes;
import org.elasticsearch.xpack.ql.type.EsField;
import org.elasticsearch.xpack.ql.util.StringUtils;

Expand Down Expand Up @@ -1413,6 +1414,18 @@ public void testPropagateEquals_VarEq2OrVarRangeGt3Lt4OrVarGt2OrVarNe2() {
assertEquals(TRUE, exp);
}

// a == 1 AND a == 2 -> nop for date/time fields
public void testPropagateEquals_ignoreDateTimeFields() {
FieldAttribute fa = getFieldAttribute("a", DataTypes.DATETIME);
Equals eq1 = equalsOf(fa, ONE);
Equals eq2 = equalsOf(fa, TWO);
And and = new And(EMPTY, eq1, eq2);

PropagateEquals rule = new PropagateEquals();
Expression exp = rule.rule(and);
assertEquals(and, exp);
}

//
// Like / Regex
//
Expand Down

0 comments on commit 8fdaae5

Please sign in to comment.