From 06eae58d40eeb92a2507620735f7622d4022e1d1 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Thu, 15 Oct 2020 14:22:28 +0300 Subject: [PATCH] EQL: Fix translation of bool fields (#63694) This commit fixes two issues in dealing with bool fields in EQL: - avoid simplifications of field == true expressions - adding comparison to clauses on fields missing logic (where bool) Fix #63693 (cherry picked from commit d10a5d0e842bbd4e0031834de948ceb24da3872b) (cherry picked from commit 0227da3a275c7f22ff524d99d53e1a79146f9e28) --- .../xpack/eql/optimizer/Optimizer.java | 40 +++++++++++++++-- .../src/test/resources/mapping-default.json | 3 ++ .../src/test/resources/queryfolder_tests.txt | 43 +++++++++++++++++++ .../function/scalar/BinaryScalarFunction.java | 7 ++- .../xpack/ql/optimizer/OptimizerRules.java | 7 +-- .../ql/optimizer/OptimizerRulesTests.java | 36 +++++++++++----- 6 files changed, 116 insertions(+), 20 deletions(-) diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java index 018c319e26e..f657a312107 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java @@ -14,6 +14,7 @@ import org.elasticsearch.xpack.eql.plan.physical.LocalRelation; import org.elasticsearch.xpack.eql.session.Payload.Type; import org.elasticsearch.xpack.eql.util.MathUtils; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.FieldAttribute; import org.elasticsearch.xpack.ql.expression.Literal; import org.elasticsearch.xpack.ql.expression.NamedExpression; import org.elasticsearch.xpack.ql.expression.Order; @@ -21,13 +22,14 @@ import org.elasticsearch.xpack.ql.expression.Order.NullsPosition; import org.elasticsearch.xpack.ql.expression.Order.OrderDirection; import org.elasticsearch.xpack.ql.expression.predicate.Predicates; import org.elasticsearch.xpack.ql.expression.predicate.logical.And; +import org.elasticsearch.xpack.ql.expression.predicate.logical.BinaryLogic; import org.elasticsearch.xpack.ql.expression.predicate.logical.Or; import org.elasticsearch.xpack.ql.expression.predicate.nulls.IsNotNull; import org.elasticsearch.xpack.ql.expression.predicate.nulls.IsNull; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NotEquals; -import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanEqualsSimplification; +import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanFunctionEqualsElimination; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineBinaryComparisons; @@ -49,10 +51,10 @@ import org.elasticsearch.xpack.ql.rule.RuleExecutor; import org.elasticsearch.xpack.ql.type.DataTypes; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import java.util.Objects; +import static java.util.Arrays.asList; import static java.util.Collections.singletonList; import static java.util.stream.Collectors.toList; @@ -68,12 +70,15 @@ public class Optimizer extends RuleExecutor { new ReplaceSurrogateFunction(), new ReplaceRegexMatch()); + Batch syntactic = new Batch("Rewrite Syntactic Sugar", Limiter.ONCE, + new AddMissingEquals()); + Batch operators = new Batch("Operator Optimization", new ConstantFolding(), // boolean new BooleanSimplification(), new BooleanLiteralsOnTheRight(), - new BooleanEqualsSimplification(), + new BooleanFunctionEqualsElimination(), // needs to occur before BinaryComparison combinations new ReplaceNullChecks(), new PropagateEquals(), @@ -99,7 +104,34 @@ public class Optimizer extends RuleExecutor { Batch label = new Batch("Set as Optimized", Limiter.ONCE, new SetAsOptimized()); - return Arrays.asList(substitutions, operators, constraints, operators, ordering, local, label); + return asList(substitutions, syntactic, operators, constraints, operators, ordering, local, label); + } + + private static class AddMissingEquals extends OptimizerRule { + + @Override + protected LogicalPlan rule(Filter filter) { + // check the condition itself + Expression condition = replaceRawBoolFieldWithEquals(filter.condition()); + // otherwise look for binary logic + if (condition == filter.condition()) { + condition = condition.transformUp(b -> + b.replaceChildren(asList(replaceRawBoolFieldWithEquals(b.left()), replaceRawBoolFieldWithEquals(b.right()))) + , BinaryLogic.class); + } + + if (condition != filter.condition()) { + filter = new Filter(filter.source(), filter.child(), condition); + } + return filter; + } + + private Expression replaceRawBoolFieldWithEquals(Expression e) { + if (e instanceof FieldAttribute) { + e = new Equals(e.source(), e, Literal.of(e, Boolean.TRUE)); + } + return e; + } } private static class ReplaceNullChecks extends OptimizerRule { diff --git a/x-pack/plugin/eql/src/test/resources/mapping-default.json b/x-pack/plugin/eql/src/test/resources/mapping-default.json index 0052eecd7a3..29a5235a03a 100644 --- a/x-pack/plugin/eql/src/test/resources/mapping-default.json +++ b/x-pack/plugin/eql/src/test/resources/mapping-default.json @@ -87,6 +87,9 @@ }, "constant_keyword" : { "type" : "constant_keyword" + }, + "bool" : { + "type" : "boolean" } } } diff --git a/x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt b/x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt index 949759a67a1..8c94410512b 100644 --- a/x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt +++ b/x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt @@ -145,6 +145,49 @@ process where endsWith(user_name, "c") {"wildcard":{"user_name":{"wildcard":"*c","boost":1.0}}}],"adjust_pure_negative":true,"boost":1.0}} ; +fieldNoEquals +process where bool +; +{"bool":{"must":[ +{"term":{"bool":{"value":true, +; + +fieldNoEqualsInExpression +process where length(file_name) > 0 and bool +; +{"bool":{"must":[ +{"term":{"bool":{"value":true, +; + + +fieldEqualsTrue +process where bool == true +; +{"bool":{"must":[ +{"term":{"bool":{"value":true, +; + +fieldEqualsFalse +process where bool == false +; +{"bool":{"must":[ +{"term":{"bool":{"value":false +; + +fieldNotEqualsTrue +process where bool != true +; +{"bool":{"must_not":[ +{"term":{"bool":{"value":true +; + +fieldNotEqualsFalse +process where bool != false +; +{"bool":{"must_not":[ +{"term":{"bool":{"value":false +; + lengthFunctionWithExactSubField process where length(file_name) > 0 ; diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/function/scalar/BinaryScalarFunction.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/function/scalar/BinaryScalarFunction.java index 9a3497cbf1b..7a156476f30 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/function/scalar/BinaryScalarFunction.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/function/scalar/BinaryScalarFunction.java @@ -31,7 +31,10 @@ public abstract class BinaryScalarFunction extends ScalarFunction { if (newChildren.size() != 2) { throw new IllegalArgumentException("expected [2] children but received [" + newChildren.size() + "]"); } - return replaceChildren(newChildren.get(0), newChildren.get(1)); + Expression newLeft = newChildren.get(0); + Expression newRight = newChildren.get(1); + + return left.equals(newLeft) && right.equals(newRight) ? this : replaceChildren(newLeft, newRight); } protected abstract BinaryScalarFunction replaceChildren(Expression newLeft, Expression newRight); @@ -63,7 +66,7 @@ public abstract class BinaryScalarFunction extends ScalarFunction { Check.isTrue(index > 0, "invalid package {}", prefix); return Scripts.binaryMethod("{" + prefix.substring(0, index) + "}", scriptMethodName(), leftScript, rightScript, dataType()); } - + protected String scriptMethodName() { return getClass().getSimpleName().toLowerCase(Locale.ROOT); } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java index 1c159964f49..2d1d46acd26 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java @@ -9,6 +9,7 @@ import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.expression.Expressions; import org.elasticsearch.xpack.ql.expression.Literal; import org.elasticsearch.xpack.ql.expression.Order; +import org.elasticsearch.xpack.ql.expression.function.Function; import org.elasticsearch.xpack.ql.expression.function.scalar.SurrogateFunction; import org.elasticsearch.xpack.ql.expression.predicate.BinaryOperator; import org.elasticsearch.xpack.ql.expression.predicate.BinaryPredicate; @@ -70,15 +71,15 @@ public final class OptimizerRules { * This rule must always be placed after {@link BooleanLiteralsOnTheRight}, since it looks at TRUE/FALSE literals' existence * on the right hand-side of the {@link Equals}/{@link NotEquals} expressions. */ - public static final class BooleanEqualsSimplification extends OptimizerExpressionRule { + public static final class BooleanFunctionEqualsElimination extends OptimizerExpressionRule { - public BooleanEqualsSimplification() { + public BooleanFunctionEqualsElimination() { super(TransformDirection.UP); } @Override protected Expression rule(Expression e) { - if (e instanceof Equals || e instanceof NotEquals) { + if ((e instanceof Equals || e instanceof NotEquals) && ((BinaryComparison) e).left() instanceof Function) { // for expression "==" or "!=" TRUE/FALSE, return the expression itself or its negated variant BinaryComparison bc = (BinaryComparison) e; diff --git a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java index 06c722a07ba..d1184b71ed6 100644 --- a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java +++ b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java @@ -24,6 +24,7 @@ import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Div; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mod; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mul; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Sub; +import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThan; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThanOrEqual; @@ -35,7 +36,7 @@ import org.elasticsearch.xpack.ql.expression.predicate.regex.Like; import org.elasticsearch.xpack.ql.expression.predicate.regex.LikePattern; import org.elasticsearch.xpack.ql.expression.predicate.regex.RLike; import org.elasticsearch.xpack.ql.expression.predicate.regex.RLikePattern; -import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanEqualsSimplification; +import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanFunctionEqualsElimination; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineBinaryComparisons; @@ -275,20 +276,33 @@ public class OptimizerRulesTests extends ESTestCase { assertEquals(expected, simplification.rule(actual)); } - public void testBoolEqualsSimplification() { - BooleanEqualsSimplification s = new BooleanEqualsSimplification(); + public void testBoolEqualsSimplificationOnExpressions() { + BooleanFunctionEqualsElimination s = new BooleanFunctionEqualsElimination(); + Expression exp = new GreaterThan(EMPTY, getFieldAttribute(), L(0), null); - assertEquals(DUMMY_EXPRESSION, s.rule(new Equals(EMPTY, DUMMY_EXPRESSION, TRUE))); - assertEquals(new Not(EMPTY, DUMMY_EXPRESSION), s.rule(new Equals(EMPTY, DUMMY_EXPRESSION, FALSE))); + assertEquals(exp, s.rule(new Equals(EMPTY, exp, TRUE))); + assertEquals(new Not(EMPTY, exp), s.rule(new Equals(EMPTY, exp, FALSE))); + } - assertEquals(new Not(EMPTY, DUMMY_EXPRESSION), s.rule(notEqualsOf(DUMMY_EXPRESSION, TRUE))); - assertEquals(DUMMY_EXPRESSION, s.rule(notEqualsOf(DUMMY_EXPRESSION, FALSE))); + public void testBoolEqualsSimplificationOnFields() { + BooleanFunctionEqualsElimination s = new BooleanFunctionEqualsElimination(); - assertEquals(NULL, s.rule(new Equals(EMPTY, NULL, TRUE))); - assertEquals(new Not(EMPTY, NULL), s.rule(new Equals(EMPTY, NULL, FALSE))); + FieldAttribute field = getFieldAttribute(); - assertEquals(new Not(EMPTY, NULL), s.rule(notEqualsOf(NULL, TRUE))); - assertEquals(NULL, s.rule(notEqualsOf(NULL, FALSE))); + List comparisons = Arrays.asList( + new Equals(EMPTY, field, TRUE), + new Equals(EMPTY, field, FALSE), + notEqualsOf(field, TRUE), + notEqualsOf(field, FALSE), + new Equals(EMPTY, NULL, TRUE), + new Equals(EMPTY, NULL, FALSE), + notEqualsOf(NULL, TRUE), + notEqualsOf(NULL, FALSE) + ); + + for (BinaryComparison comparison : comparisons) { + assertEquals(comparison, s.rule(comparison)); + } } //