Skip to content

Commit

Permalink
SQL: Fix issue with date range queries and timezone (#56115)
Browse files Browse the repository at this point in the history
Previously, the timezone parameter was not passed to the RangeQuery
and as a results queries that use the ES date math notation (now,
now-1d, now/d, now/h, now+2h, etc.) were using the UTC timezone and
not the one passed through the "timezone"/"time_zone" JDBC/REST params.
As a consequence, the date math defined dates were always considered in
UTC and possibly led to incorrect results for queries like:
```
SELECT * FROM t WHERE date BETWEEN now-1d/d AND now/d
```

Fixes: #56049
  • Loading branch information
matriv authored May 5, 2020
1 parent 86032ac commit 300f010
Show file tree
Hide file tree
Showing 35 changed files with 667 additions and 432 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.function.Function;

import static java.lang.String.format;
import static org.elasticsearch.xpack.ql.type.DateUtils.UTC;

public class EqlParser {

Expand All @@ -41,7 +42,7 @@ public class EqlParser {
* Parses an EQL statement into execution plan
*/
public LogicalPlan createStatement(String eql) {
return createStatement(eql, new ParserParams());
return createStatement(eql, new ParserParams(UTC));
}

public LogicalPlan createStatement(String eql, ParserParams params) {
Expand All @@ -52,7 +53,7 @@ public LogicalPlan createStatement(String eql, ParserParams params) {
}

public Expression createExpression(String expression) {
return createExpression(expression, new ParserParams());
return createExpression(expression, new ParserParams(UTC));
}

public Expression createExpression(String expression, ParserParams params) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,20 @@
import org.elasticsearch.xpack.ql.type.DataTypes;
import org.elasticsearch.xpack.ql.util.StringUtils;

import java.time.ZoneId;
import java.util.List;

import static java.util.Collections.emptyList;


public class ExpressionBuilder extends IdentifierBuilder {

protected final ParserParams params;

public ExpressionBuilder(ParserParams params) {
this.params = params;
}

protected Expression expression(ParseTree ctx) {
return typedParsing(ctx, Expression.class);
}
Expand Down Expand Up @@ -115,20 +122,21 @@ public Expression visitComparison(ComparisonContext ctx) {
TerminalNode op = (TerminalNode) ctx.comparisonOperator().getChild(0);

Source source = source(ctx);
ZoneId zoneId = params.zoneId();

switch (op.getSymbol().getType()) {
case EqlBaseParser.EQ:
return new Equals(source, left, right);
return new Equals(source, left, right, zoneId);
case EqlBaseParser.NEQ:
return new NotEquals(source, left, right);
return new NotEquals(source, left, right, zoneId);
case EqlBaseParser.LT:
return new LessThan(source, left, right);
return new LessThan(source, left, right, zoneId);
case EqlBaseParser.LTE:
return new LessThanOrEqual(source, left, right);
return new LessThanOrEqual(source, left, right, zoneId);
case EqlBaseParser.GT:
return new GreaterThan(source, left, right);
return new GreaterThan(source, left, right, zoneId);
case EqlBaseParser.GTE:
return new GreaterThanOrEqual(source, left, right);
return new GreaterThanOrEqual(source, left, right, zoneId);
default:
throw new ParsingException(source, "Unknown operator {}", source.text());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,10 @@

public abstract class LogicalPlanBuilder extends ExpressionBuilder {

private final ParserParams params;
private final UnresolvedRelation RELATION = new UnresolvedRelation(Source.EMPTY, null, "", false, "");

public LogicalPlanBuilder(ParserParams params) {
this.params = params;
super(params);
}

@Override
Expand All @@ -56,7 +55,7 @@ public LogicalPlan visitEventQuery(EqlBaseParser.EventQueryContext ctx) {
Literal eventValue = new Literal(eventSource, eventName, DataTypes.KEYWORD);

UnresolvedAttribute eventField = new UnresolvedAttribute(eventSource, params.fieldEventCategory());
Expression eventMatch = new Equals(eventSource, eventField, eventValue);
Expression eventMatch = new Equals(eventSource, eventField, eventValue, params.zoneId());

condition = new And(source, eventMatch, condition);
}
Expand Down Expand Up @@ -214,4 +213,4 @@ public TimeValue visitSequenceParams(SequenceParamsContext ctx) {
text(numberCtx));
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

package org.elasticsearch.xpack.eql.parser;

import java.time.ZoneId;
import java.util.List;

import static java.util.Collections.emptyList;
Expand All @@ -15,10 +16,15 @@

public class ParserParams {

private final ZoneId zoneId;
private String fieldEventCategory = FIELD_EVENT_CATEGORY;
private String fieldTimestamp = FIELD_TIMESTAMP;
private String implicitJoinKey = FIELD_IMPLICIT_JOIN_KEY;
private List<Object> queryParams = emptyList();

public ParserParams(ZoneId zoneId) {
this.zoneId = zoneId;
}

public String fieldEventCategory() {
return fieldEventCategory;
Expand Down Expand Up @@ -55,4 +61,8 @@ public ParserParams params(List<Object> params) {
this.queryParams = params;
return this;
}

public ZoneId zoneId() {
return zoneId;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public static void operation(PlanExecutor planExecutor, EqlSearchTask task, EqlS
boolean includeFrozen = request.indicesOptions().ignoreThrottled() == false;
String clientId = null;

ParserParams params = new ParserParams()
ParserParams params = new ParserParams(zoneId)
.fieldEventCategory(request.eventCategoryField())
.fieldTimestamp(request.timestampField())
.implicitJoinKey(request.implicitJoinKeyField());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.List;

import static org.elasticsearch.xpack.eql.parser.AbstractBuilder.unquoteString;
import static org.elasticsearch.xpack.ql.TestUtils.UTC;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
Expand Down Expand Up @@ -144,12 +145,12 @@ public void testComparison() {
Expression field = expr(fieldText);
Expression value = expr(valueText);

assertEquals(new Equals(null, field, value), expr(fieldText + "==" + valueText));
assertEquals(new NotEquals(null, field, value), expr(fieldText + "!=" + valueText));
assertEquals(new LessThanOrEqual(null, field, value), expr(fieldText + "<=" + valueText));
assertEquals(new GreaterThanOrEqual(null, field, value), expr(fieldText + ">=" + valueText));
assertEquals(new GreaterThan(null, field, value), expr(fieldText + ">" + valueText));
assertEquals(new LessThan(null, field, value), expr(fieldText + "<" + valueText));
assertEquals(new Equals(null, field, value, UTC), expr(fieldText + "==" + valueText));
assertEquals(new NotEquals(null, field, value, UTC), expr(fieldText + "!=" + valueText));
assertEquals(new LessThanOrEqual(null, field, value, UTC), expr(fieldText + "<=" + valueText));
assertEquals(new GreaterThanOrEqual(null, field, value, UTC), expr(fieldText + ">=" + valueText));
assertEquals(new GreaterThan(null, field, value, UTC), expr(fieldText + ">" + valueText));
assertEquals(new LessThan(null, field, value, UTC), expr(fieldText + "<" + valueText));
}

public void testBoolean() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.concurrent.TimeUnit;

import static java.util.Collections.singletonList;
import static org.elasticsearch.xpack.ql.type.DateUtils.UTC;

public class LogicalPlanTests extends ESTestCase {

Expand Down Expand Up @@ -56,7 +57,7 @@ public void testEventQuery() {
}

public void testParameterizedEventQuery() {
ParserParams params = new ParserParams().fieldEventCategory("myCustomEvent");
ParserParams params = new ParserParams(UTC).fieldEventCategory("myCustomEvent");
LogicalPlan fullQuery = parser.createStatement("process where process_name == 'net.exe'", params);
Expression fullExpression = expr("myCustomEvent == 'process' and process_name == 'net.exe'");

Expand Down Expand Up @@ -126,4 +127,4 @@ public void testSequencePlan() {
assertEquals(new TimeValue(2, TimeUnit.SECONDS), maxSpan);

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.elasticsearch.xpack.ql.type.DataType;
import org.elasticsearch.xpack.ql.type.DataTypes;

import java.time.ZoneId;
import java.util.List;
import java.util.Locale;
import java.util.Objects;
Expand All @@ -35,28 +36,30 @@ public class Range extends ScalarFunction {

private final Expression value, lower, upper;
private final boolean includeLower, includeUpper;
private final ZoneId zoneId;

public Range(Source source, Expression value, Expression lower, boolean includeLower, Expression upper, boolean includeUpper) {
super(source, asList(value, lower, upper));
public Range(Source src, Expression value, Expression lower, boolean inclLower, Expression upper, boolean inclUpper, ZoneId zoneId) {
super(src, asList(value, lower, upper));

this.value = value;
this.lower = lower;
this.upper = upper;
this.includeLower = includeLower;
this.includeUpper = includeUpper;
this.includeLower = inclLower;
this.includeUpper = inclUpper;
this.zoneId = zoneId;
}

@Override
protected NodeInfo<Range> info() {
return NodeInfo.create(this, Range::new, value, lower, includeLower, upper, includeUpper);
return NodeInfo.create(this, Range::new, value, lower, includeLower, upper, includeUpper, zoneId);
}

@Override
public Expression replaceChildren(List<Expression> newChildren) {
if (newChildren.size() != 3) {
throw new IllegalArgumentException("expected [3] children but received [" + newChildren.size() + "]");
}
return new Range(source(), newChildren.get(0), newChildren.get(1), includeLower, newChildren.get(2), includeUpper);
return new Range(source(), newChildren.get(0), newChildren.get(1), includeLower, newChildren.get(2), includeUpper, zoneId);
}

public Expression value() {
Expand All @@ -79,6 +82,10 @@ public boolean includeUpper() {
return includeUpper;
}

public ZoneId zoneId() {
return zoneId;
}

@Override
public boolean foldable() {
if (lower.foldable() && upper.foldable()) {
Expand Down Expand Up @@ -160,7 +167,7 @@ protected Pipe makePipe() {

@Override
public int hashCode() {
return Objects.hash(includeLower, includeUpper, value, lower, upper);
return Objects.hash(includeLower, includeUpper, value, lower, upper, zoneId);
}

@Override
Expand All @@ -178,6 +185,7 @@ public boolean equals(Object obj) {
&& Objects.equals(includeUpper, other.includeUpper)
&& Objects.equals(value, other.value)
&& Objects.equals(lower, other.lower)
&& Objects.equals(upper, other.upper);
&& Objects.equals(upper, other.upper)
&& Objects.equals(zoneId, other.zoneId);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,20 @@
import org.elasticsearch.xpack.ql.type.DataType;
import org.elasticsearch.xpack.ql.type.DataTypes;

import java.time.ZoneId;

// marker class to indicate operations that rely on values
public abstract class BinaryComparison extends BinaryOperator<Object, Object, Boolean, BinaryComparisonOperation> {

protected BinaryComparison(Source source, Expression left, Expression right, BinaryComparisonOperation operation) {
private final ZoneId zoneId;

protected BinaryComparison(Source source, Expression left, Expression right, BinaryComparisonOperation operation, ZoneId zoneId) {
super(source, left, right, operation);
this.zoneId = zoneId;
}

public ZoneId zoneId() {
return zoneId;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,35 @@
import org.elasticsearch.xpack.ql.tree.NodeInfo;
import org.elasticsearch.xpack.ql.tree.Source;

import java.time.ZoneId;

public class Equals extends BinaryComparison implements Negatable<BinaryComparison> {

public Equals(Source source, Expression left, Expression right) {
super(source, left, right, BinaryComparisonOperation.EQ);
super(source, left, right, BinaryComparisonOperation.EQ, null);
}

public Equals(Source source, Expression left, Expression right, ZoneId zoneId) {
super(source, left, right, BinaryComparisonOperation.EQ, zoneId);
}

@Override
protected NodeInfo<Equals> info() {
return NodeInfo.create(this, Equals::new, left(), right());
return NodeInfo.create(this, Equals::new, left(), right(), zoneId());
}

@Override
protected Equals replaceChildren(Expression newLeft, Expression newRight) {
return new Equals(source(), newLeft, newRight);
return new Equals(source(), newLeft, newRight, zoneId());
}

@Override
public Equals swapLeftAndRight() {
return new Equals(source(), right(), left());
return new Equals(source(), right(), left(), zoneId());
}

@Override
public BinaryComparison negate() {
return new NotEquals(source(), left(), right());
return new NotEquals(source(), left(), right(), zoneId());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,31 @@
import org.elasticsearch.xpack.ql.tree.NodeInfo;
import org.elasticsearch.xpack.ql.tree.Source;

import java.time.ZoneId;

public class GreaterThan extends BinaryComparison implements Negatable<BinaryComparison> {

public GreaterThan(Source source, Expression left, Expression right) {
super(source, left, right, BinaryComparisonOperation.GT);
public GreaterThan(Source source, Expression left, Expression right, ZoneId zoneId) {
super(source, left, right, BinaryComparisonOperation.GT, zoneId);
}

@Override
protected NodeInfo<GreaterThan> info() {
return NodeInfo.create(this, GreaterThan::new, left(), right());
return NodeInfo.create(this, GreaterThan::new, left(), right(), zoneId());
}

@Override
protected GreaterThan replaceChildren(Expression newLeft, Expression newRight) {
return new GreaterThan(source(), newLeft, newRight);
return new GreaterThan(source(), newLeft, newRight, zoneId());
}

@Override
public LessThan swapLeftAndRight() {
return new LessThan(source(), right(), left());
return new LessThan(source(), right(), left(), zoneId());
}

@Override
public LessThanOrEqual negate() {
return new LessThanOrEqual(source(), left(), right());
return new LessThanOrEqual(source(), left(), right(), zoneId());
}
}
Loading

0 comments on commit 300f010

Please sign in to comment.