From 97352a3a9c3843e6da140ce93438ddca194ebd6f Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Wed, 6 Dec 2023 21:20:51 -0800 Subject: [PATCH 1/5] ESQL: Simpify IS NULL/IS NOT NULL evaluation The IS NULL, IS NOT NULL predicates only care about the nullability not the value of an expression. Given the expression x + 1 / 2, the actual result does not matter only if it's null or not - that is, it only matters if x is null or not. So x + 1 / 2 IS NULL becomes x IS NULL - which can be opportunistically pushed down or evaluated. Fix #103097 --- .../esql/optimizer/LogicalPlanOptimizer.java | 12 +- .../elasticsearch/xpack/esql/CsvTests.java | 2 +- .../xpack/ql/optimizer/OptimizerRules.java | 105 ++++++++++++++++++ .../ql/optimizer/OptimizerRulesTests.java | 76 +++++++++++++ 4 files changed, 193 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java index 66654b78c3af4..c483091d4242e 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java @@ -16,6 +16,7 @@ import org.elasticsearch.xpack.esql.evaluator.predicate.operator.comparison.Equals; import org.elasticsearch.xpack.esql.expression.SurrogateExpression; import org.elasticsearch.xpack.esql.expression.function.aggregate.Count; +import org.elasticsearch.xpack.esql.expression.function.scalar.nulls.Coalesce; import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.In; import org.elasticsearch.xpack.esql.plan.logical.Enrich; import org.elasticsearch.xpack.esql.plan.logical.Eval; @@ -107,8 +108,9 @@ protected static List> rules() { Limiter.ONCE, new SubstituteSurrogates(), new ReplaceRegexMatch(), - new ReplaceAliasingEvalWithProject() + new ReplaceAliasingEvalWithProject(), // new NormalizeAggregate(), - waits on https://github.com/elastic/elasticsearch/issues/100634 + new NullableSimplification() ); var operators = new Batch<>( @@ -1137,6 +1139,14 @@ private LogicalPlan rule(Eval eval) { } } + static class NullableSimplification extends OptimizerRules.NullableSimplification { + + @Override + protected boolean skipExpression(Expression e) { + return e instanceof Coalesce; + } + } + private abstract static class ParameterizedOptimizerRule extends ParameterizedRule< SubPlan, LogicalPlan, diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java index 17ed0c1223636..a5e3033b6e1e3 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java @@ -229,7 +229,7 @@ protected final boolean enableWarningsCheck() { } public boolean logResults() { - return true; + return false; } private void doTest() throws Exception { 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 5d9736726b46f..7f2489edafb9a 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 @@ -8,6 +8,8 @@ import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.xpack.ql.expression.Alias; +import org.elasticsearch.xpack.ql.expression.Attribute; +import org.elasticsearch.xpack.ql.expression.AttributeMap; import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.expression.Expressions; import org.elasticsearch.xpack.ql.expression.Literal; @@ -17,6 +19,7 @@ import org.elasticsearch.xpack.ql.expression.function.Functions; import org.elasticsearch.xpack.ql.expression.function.scalar.ScalarFunction; import org.elasticsearch.xpack.ql.expression.function.scalar.SurrogateFunction; +import org.elasticsearch.xpack.ql.expression.function.scalar.UnaryScalarFunction; import org.elasticsearch.xpack.ql.expression.predicate.BinaryOperator; import org.elasticsearch.xpack.ql.expression.predicate.BinaryPredicate; import org.elasticsearch.xpack.ql.expression.predicate.Negatable; @@ -1785,6 +1788,108 @@ protected Expression nonNullify(Expression exp, Expression nonNullExp) { } } + /** + * Simplify IsNull/IsNotNull targets by resolving the underlying expression to its root fields with unknown + * nullability. + * e.g. + * (x + 1) / 2 IS NULL --> x IS NULL + * SUBSTRING(x, 3) > 4 IS NULL --> x IS NULL + * When dealing with multiple fields, the a conjunction/disjunction based on the predicate: + * (x + y) / 4 IS NULL --> x IS NULL OR x IS NULL + * (x + y) / 4 IS NOT NULL --> x IS NOT NULL AND y IS NOT NULL + * This handles the case of fields nested inside functions or expressions in order to avoid: + * - having to evaluate the whole expression + * - not pushing down the filter due to expression evaluation + *
+ * Implementation-wise this rule goes bottom-up, keeping an alias up to date to the current plan + * and then looks for replacing the target of the IS NULL / IS NOT NULL expressions accordingly. + */ + public static class NullableSimplification extends Rule { + + @Override + public LogicalPlan apply(LogicalPlan plan) { + // the alias map is shared across the whole plan + AttributeMap aliases = new AttributeMap<>(); + // traverse bottom-up to pick up the aliases as we go + plan = plan.transformUp(p -> inspectPlan(p, aliases)); + return plan; + } + + private LogicalPlan inspectPlan(LogicalPlan plan, AttributeMap aliases) { + // inspect just this plan properties + plan.forEachExpression(Alias.class, a -> aliases.put(a.toAttribute(), a.child())); + // now go about finding isNull/isNotNull + LogicalPlan newPlan = plan.transformExpressionsOnlyUp(UnaryScalarFunction.class, f -> simplifyExpression(f, aliases)); + return newPlan; + } + + private Expression simplifyExpression(UnaryScalarFunction function, AttributeMap aliases) { + Expression result = function; + if (function instanceof IsNotNull || function instanceof IsNull) { + result = simplifyNullable(function, aliases); + } + + return result; + } + + private Expression simplifyNullable(UnaryScalarFunction function, AttributeMap aliases) { + Expression result = function; + Set refs = resolveExpressionAsRootAttributes(function.field(), aliases); + // no refs found or could not detect - return the original function + if (refs.size() > 0) { + boolean isNotNull = function instanceof IsNotNull; + + java.util.function.Function, Expression> combiner = isNotNull + ? Predicates::combineAnd + : Predicates::combineOr; + + java.util.function.Function constructor = e -> isNotNull + ? new IsNotNull(function.source(), e) + : new IsNull(function.source(), e); + + // convert the list to a set + result = combiner.apply(refs.stream().map(constructor).toList()); + } + return result; + } + + /** + * Unroll the expression to its references to get to the root fields + * that really matter for filtering. + */ + protected Set resolveExpressionAsRootAttributes(Expression exp, AttributeMap aliases) { + Set resolvedExpressions = new LinkedHashSet<>(); + doResolve(exp, aliases, resolvedExpressions); + return resolvedExpressions; + } + + private void doResolve(Expression exp, AttributeMap aliases, Set resolvedExpressions) { + if (skipExpression(exp)) { + resolvedExpressions.add(exp); + } else { + // expression might be nullable, go after it + if (exp.nullable() != Nullability.FALSE) { + for (Expression e : exp.references()) { + Expression resolved = aliases.resolve(e, e); + // found a root attribute, bail out + if (resolved instanceof Attribute a && resolved == e) { + resolvedExpressions.add(a); + } else { + // go further + doResolve(resolved, aliases, resolvedExpressions); + } + } + } else { + resolvedExpressions.add(exp); + } + } + } + + protected boolean skipExpression(Expression e) { + return false; + } + } + public static final class SetAsOptimized extends Rule { @Override 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 b14e46a96a9e6..9893866c296b1 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 @@ -14,6 +14,7 @@ import org.elasticsearch.xpack.ql.expression.Literal; import org.elasticsearch.xpack.ql.expression.Nullability; import org.elasticsearch.xpack.ql.expression.function.aggregate.Count; +import org.elasticsearch.xpack.ql.expression.function.scalar.string.StartsWith; import org.elasticsearch.xpack.ql.expression.predicate.BinaryOperator; import org.elasticsearch.xpack.ql.expression.predicate.Predicates; import org.elasticsearch.xpack.ql.expression.predicate.Range; @@ -1771,6 +1772,81 @@ public void testPushDownFilterThroughAgg() throws Exception { } + public void testIsNotNullOnOperatorWithOneField() { + EsRelation relation = relation(); + var fieldA = getFieldAttribute("a"); + Expression inn = isNotNull(new Add(EMPTY, fieldA, ONE)); + Filter f = new Filter(EMPTY, relation, inn); + Filter expected = new Filter(EMPTY, relation, isNotNull(fieldA)); + + assertEquals(expected, new OptimizerRules.NullableSimplification().apply(f)); + } + + public void testIsNotNullOnOperatorWithTwoFields() { + EsRelation relation = relation(); + var fieldA = getFieldAttribute("a"); + var fieldB = getFieldAttribute("b"); + Expression inn = isNotNull(new Add(EMPTY, fieldA, fieldB)); + Filter f = new Filter(EMPTY, relation, inn); + Filter expected = new Filter(EMPTY, relation, new And(EMPTY, isNotNull(fieldA), isNotNull(fieldB))); + + assertEquals(expected, new OptimizerRules.NullableSimplification().apply(f)); + } + + public void testIsNotNullOnFunctionWithOneField() { + EsRelation relation = relation(); + var fieldA = getFieldAttribute("a"); + var pattern = L("abc"); + Expression inn = isNotNull( + new And(EMPTY, new TestStartsWith(EMPTY, fieldA, pattern, false), greaterThanOf(new Add(EMPTY, ONE, TWO), THREE)) + ); + + Filter f = new Filter(EMPTY, relation, inn); + Filter expected = new Filter(EMPTY, relation, isNotNull(fieldA)); + + assertEquals(expected, new OptimizerRules.NullableSimplification().apply(f)); + } + + public void testIsNotNullOnFunctionWithTwoFields() { + EsRelation relation = relation(); + var fieldA = getFieldAttribute("a"); + var fieldB = getFieldAttribute("b"); + var pattern = L("abc"); + Expression inn = isNotNull(new TestStartsWith(EMPTY, fieldA, fieldB, false)); + + Filter f = new Filter(EMPTY, relation, inn); + Filter expected = new Filter(EMPTY, relation, new And(EMPTY, isNotNull(fieldA), isNotNull(fieldB))); + + assertEquals(expected, new OptimizerRules.NullableSimplification().apply(f)); + } + + public static class TestStartsWith extends StartsWith { + + public TestStartsWith(Source source, Expression input, Expression pattern, boolean caseInsensitive) { + super(source, input, pattern, caseInsensitive); + } + + @Override + public Expression replaceChildren(List newChildren) { + return new TestStartsWith(source(), newChildren.get(0), newChildren.get(1), isCaseInsensitive()); + } + + @Override + protected NodeInfo info() { + return NodeInfo.create(this, TestStartsWith::new, input(), pattern(), isCaseInsensitive()); + } + } + + public void testIsNotNullOnFunctionWithTwoField() {} + + private IsNotNull isNotNull(Expression field) { + return new IsNotNull(EMPTY, field); + } + + private IsNull isNull(Expression field) { + return new IsNull(EMPTY, field); + } + private Literal nullOf(DataType dataType) { return new Literal(Source.EMPTY, null, dataType); } From 5a009dd67dad75d6ca09f54089d26f200d386787 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Thu, 7 Dec 2023 17:52:43 -0800 Subject: [PATCH 2/5] Move the rule to the local optimizer Preserve the original expression to cope with under/overflow or mv fields --- .../optimizer/LocalLogicalPlanOptimizer.java | 18 ++++++- .../esql/optimizer/LogicalPlanOptimizer.java | 12 +---- .../xpack/ql/optimizer/OptimizerRules.java | 52 +++++++------------ .../ql/optimizer/OptimizerRulesTests.java | 17 +++--- 4 files changed, 44 insertions(+), 55 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java index 3451a3981d3e3..e05dd9a00c567 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java @@ -7,10 +7,12 @@ package org.elasticsearch.xpack.esql.optimizer; +import org.elasticsearch.xpack.esql.expression.function.scalar.nulls.Coalesce; import org.elasticsearch.xpack.esql.plan.logical.Eval; import org.elasticsearch.xpack.esql.plan.logical.TopN; import org.elasticsearch.xpack.esql.stats.SearchStats; import org.elasticsearch.xpack.ql.expression.Alias; +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; @@ -37,7 +39,13 @@ public LocalLogicalPlanOptimizer(LocalLogicalOptimizerContext localLogicalOptimi @Override protected List> batches() { - var local = new Batch<>("Local rewrite", new ReplaceTopNWithLimitAndSort(), new ReplaceMissingFieldWithNull()); + var local = new Batch<>( + "Local rewrite", + Limiter.ONCE, + new ReplaceTopNWithLimitAndSort(), + new ReplaceMissingFieldWithNull(), + new InferIsNotNull() + ); var rules = new ArrayList>(); rules.add(local); @@ -116,6 +124,14 @@ else if (plan instanceof Project project) { } } + static class InferIsNotNull extends OptimizerRules.InferIsNotNull { + + @Override + protected boolean skipExpression(Expression e) { + return e instanceof Coalesce; + } + } + abstract static class ParameterizedOptimizerRule extends ParameterizedRule { public final LogicalPlan apply(LogicalPlan plan, P context) { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java index c483091d4242e..66654b78c3af4 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java @@ -16,7 +16,6 @@ import org.elasticsearch.xpack.esql.evaluator.predicate.operator.comparison.Equals; import org.elasticsearch.xpack.esql.expression.SurrogateExpression; import org.elasticsearch.xpack.esql.expression.function.aggregate.Count; -import org.elasticsearch.xpack.esql.expression.function.scalar.nulls.Coalesce; import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.In; import org.elasticsearch.xpack.esql.plan.logical.Enrich; import org.elasticsearch.xpack.esql.plan.logical.Eval; @@ -108,9 +107,8 @@ protected static List> rules() { Limiter.ONCE, new SubstituteSurrogates(), new ReplaceRegexMatch(), - new ReplaceAliasingEvalWithProject(), + new ReplaceAliasingEvalWithProject() // new NormalizeAggregate(), - waits on https://github.com/elastic/elasticsearch/issues/100634 - new NullableSimplification() ); var operators = new Batch<>( @@ -1139,14 +1137,6 @@ private LogicalPlan rule(Eval eval) { } } - static class NullableSimplification extends OptimizerRules.NullableSimplification { - - @Override - protected boolean skipExpression(Expression e) { - return e instanceof Coalesce; - } - } - private abstract static class ParameterizedOptimizerRule extends ParameterizedRule< SubPlan, LogicalPlan, 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 7f2489edafb9a..1c607b24dab70 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 @@ -19,7 +19,6 @@ import org.elasticsearch.xpack.ql.expression.function.Functions; import org.elasticsearch.xpack.ql.expression.function.scalar.ScalarFunction; import org.elasticsearch.xpack.ql.expression.function.scalar.SurrogateFunction; -import org.elasticsearch.xpack.ql.expression.function.scalar.UnaryScalarFunction; import org.elasticsearch.xpack.ql.expression.predicate.BinaryOperator; import org.elasticsearch.xpack.ql.expression.predicate.BinaryPredicate; import org.elasticsearch.xpack.ql.expression.predicate.Negatable; @@ -1789,22 +1788,25 @@ protected Expression nonNullify(Expression exp, Expression nonNullExp) { } /** - * Simplify IsNull/IsNotNull targets by resolving the underlying expression to its root fields with unknown + * Simplify IsNotNull targets by resolving the underlying expression to its root fields with unknown * nullability. * e.g. - * (x + 1) / 2 IS NULL --> x IS NULL - * SUBSTRING(x, 3) > 4 IS NULL --> x IS NULL - * When dealing with multiple fields, the a conjunction/disjunction based on the predicate: - * (x + y) / 4 IS NULL --> x IS NULL OR x IS NULL - * (x + y) / 4 IS NOT NULL --> x IS NOT NULL AND y IS NOT NULL + * (x + 1) / 2 IS NOT NULL --> x IS NOT NULL AND (x+1) / 2 IS NOT NULL + * SUBSTRING(x, 3) > 4 IS NOT NULL --> x IS NOT NULL AND SUBSTRING(x, 3) > 4 IS NOT NULL + * When dealing with multiple fields, a conjunction/disjunction based on the predicate: + * (x + y) / 4 IS NOT NULL --> x IS NOT NULL AND y IS NOT NULL AND (x + y) / 4 IS NOT NULL * This handles the case of fields nested inside functions or expressions in order to avoid: * - having to evaluate the whole expression * - not pushing down the filter due to expression evaluation + * IS NULL cannot be simplified since it leads to a disjunction which cannot prevents the simplified IS NULL + * to be pushed down: + * (x + 1) IS NULL --> x IS NULL OR x + 1 IS NULL + * and x IS NULL cannot be pushed down *
* Implementation-wise this rule goes bottom-up, keeping an alias up to date to the current plan - * and then looks for replacing the target of the IS NULL / IS NOT NULL expressions accordingly. + * and then looks for replacing the target. */ - public static class NullableSimplification extends Rule { + public static class InferIsNotNull extends Rule { @Override public LogicalPlan apply(LogicalPlan plan) { @@ -1819,36 +1821,18 @@ private LogicalPlan inspectPlan(LogicalPlan plan, AttributeMap alias // inspect just this plan properties plan.forEachExpression(Alias.class, a -> aliases.put(a.toAttribute(), a.child())); // now go about finding isNull/isNotNull - LogicalPlan newPlan = plan.transformExpressionsOnlyUp(UnaryScalarFunction.class, f -> simplifyExpression(f, aliases)); + LogicalPlan newPlan = plan.transformExpressionsOnlyUp(IsNotNull.class, inn -> inferNotNullable(inn, aliases)); return newPlan; } - private Expression simplifyExpression(UnaryScalarFunction function, AttributeMap aliases) { - Expression result = function; - if (function instanceof IsNotNull || function instanceof IsNull) { - result = simplifyNullable(function, aliases); - } - - return result; - } - - private Expression simplifyNullable(UnaryScalarFunction function, AttributeMap aliases) { - Expression result = function; - Set refs = resolveExpressionAsRootAttributes(function.field(), aliases); + private Expression inferNotNullable(IsNotNull inn, AttributeMap aliases) { + Expression result = inn; + Set refs = resolveExpressionAsRootAttributes(inn.field(), aliases); // no refs found or could not detect - return the original function if (refs.size() > 0) { - boolean isNotNull = function instanceof IsNotNull; - - java.util.function.Function, Expression> combiner = isNotNull - ? Predicates::combineAnd - : Predicates::combineOr; - - java.util.function.Function constructor = e -> isNotNull - ? new IsNotNull(function.source(), e) - : new IsNull(function.source(), e); - - // convert the list to a set - result = combiner.apply(refs.stream().map(constructor).toList()); + // add IsNull for the filters along with the initial inn + var innList = CollectionUtils.combine(refs.stream().map(r -> (Expression) new IsNotNull(inn.source(), r)).toList(), inn); + result = Predicates.combineAnd(innList); } return result; } 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 9893866c296b1..087ceb525be0c 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 @@ -1769,7 +1769,6 @@ public void testPushDownFilterThroughAgg() throws Exception { // expected Filter expected = new Filter(EMPTY, new Aggregate(EMPTY, combinedFilter, emptyList(), emptyList()), aggregateCondition); assertEquals(expected, new PushDownAndCombineFilters().apply(fb)); - } public void testIsNotNullOnOperatorWithOneField() { @@ -1777,9 +1776,9 @@ public void testIsNotNullOnOperatorWithOneField() { var fieldA = getFieldAttribute("a"); Expression inn = isNotNull(new Add(EMPTY, fieldA, ONE)); Filter f = new Filter(EMPTY, relation, inn); - Filter expected = new Filter(EMPTY, relation, isNotNull(fieldA)); + Filter expected = new Filter(EMPTY, relation, new And(EMPTY, isNotNull(fieldA), inn)); - assertEquals(expected, new OptimizerRules.NullableSimplification().apply(f)); + assertEquals(expected, new OptimizerRules.InferIsNotNull().apply(f)); } public void testIsNotNullOnOperatorWithTwoFields() { @@ -1788,9 +1787,9 @@ public void testIsNotNullOnOperatorWithTwoFields() { var fieldB = getFieldAttribute("b"); Expression inn = isNotNull(new Add(EMPTY, fieldA, fieldB)); Filter f = new Filter(EMPTY, relation, inn); - Filter expected = new Filter(EMPTY, relation, new And(EMPTY, isNotNull(fieldA), isNotNull(fieldB))); + Filter expected = new Filter(EMPTY, relation, new And(EMPTY, new And(EMPTY, isNotNull(fieldA), isNotNull(fieldB)), inn)); - assertEquals(expected, new OptimizerRules.NullableSimplification().apply(f)); + assertEquals(expected, new OptimizerRules.InferIsNotNull().apply(f)); } public void testIsNotNullOnFunctionWithOneField() { @@ -1802,9 +1801,9 @@ public void testIsNotNullOnFunctionWithOneField() { ); Filter f = new Filter(EMPTY, relation, inn); - Filter expected = new Filter(EMPTY, relation, isNotNull(fieldA)); + Filter expected = new Filter(EMPTY, relation, new And(EMPTY, isNotNull(fieldA), inn)); - assertEquals(expected, new OptimizerRules.NullableSimplification().apply(f)); + assertEquals(expected, new OptimizerRules.InferIsNotNull().apply(f)); } public void testIsNotNullOnFunctionWithTwoFields() { @@ -1815,9 +1814,9 @@ public void testIsNotNullOnFunctionWithTwoFields() { Expression inn = isNotNull(new TestStartsWith(EMPTY, fieldA, fieldB, false)); Filter f = new Filter(EMPTY, relation, inn); - Filter expected = new Filter(EMPTY, relation, new And(EMPTY, isNotNull(fieldA), isNotNull(fieldB))); + Filter expected = new Filter(EMPTY, relation, new And(EMPTY, new And(EMPTY, isNotNull(fieldA), isNotNull(fieldB)), inn)); - assertEquals(expected, new OptimizerRules.NullableSimplification().apply(f)); + assertEquals(expected, new OptimizerRules.InferIsNotNull().apply(f)); } public static class TestStartsWith extends StartsWith { From c4c70ae2f27bef67faaed7908e763b760673e326 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Thu, 7 Dec 2023 17:54:38 -0800 Subject: [PATCH 3/5] Update docs/changelog/103099.yaml --- docs/changelog/103099.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/103099.yaml diff --git a/docs/changelog/103099.yaml b/docs/changelog/103099.yaml new file mode 100644 index 0000000000000..c3fd3f9d7b8e4 --- /dev/null +++ b/docs/changelog/103099.yaml @@ -0,0 +1,6 @@ +pr: 103099 +summary: "ESQL: Simpify IS NULL/IS NOT NULL evaluation" +area: ES|QL +type: enhancement +issues: + - 103097 From d67c7090b3eb1ea0fb41809b47798d9c10cac954 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Mon, 11 Dec 2023 20:49:44 -0800 Subject: [PATCH 4/5] Reduce creation of duplicates --- .../xpack/ql/optimizer/OptimizerRules.java | 39 ++++++++++--------- .../ql/optimizer/OptimizerRulesTests.java | 9 +++++ 2 files changed, 29 insertions(+), 19 deletions(-) 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 1c607b24dab70..f084b5cda4abe 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 @@ -71,6 +71,7 @@ import static java.lang.Math.signum; import static java.util.Arrays.asList; +import static java.util.Collections.emptySet; import static org.elasticsearch.xpack.ql.expression.Literal.FALSE; import static org.elasticsearch.xpack.ql.expression.Literal.TRUE; import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.combineAnd; @@ -1798,8 +1799,8 @@ protected Expression nonNullify(Expression exp, Expression nonNullExp) { * This handles the case of fields nested inside functions or expressions in order to avoid: * - having to evaluate the whole expression * - not pushing down the filter due to expression evaluation - * IS NULL cannot be simplified since it leads to a disjunction which cannot prevents the simplified IS NULL - * to be pushed down: + * IS NULL cannot be simplified since it leads to a disjunction which prevents the filter to be + * pushed down: * (x + 1) IS NULL --> x IS NULL OR x + 1 IS NULL * and x IS NULL cannot be pushed down *
@@ -1843,30 +1844,30 @@ private Expression inferNotNullable(IsNotNull inn, AttributeMap alia */ protected Set resolveExpressionAsRootAttributes(Expression exp, AttributeMap aliases) { Set resolvedExpressions = new LinkedHashSet<>(); - doResolve(exp, aliases, resolvedExpressions); - return resolvedExpressions; + boolean changed = doResolve(exp, aliases, resolvedExpressions); + return changed ? resolvedExpressions : emptySet(); } - private void doResolve(Expression exp, AttributeMap aliases, Set resolvedExpressions) { - if (skipExpression(exp)) { + private boolean doResolve(Expression exp, AttributeMap aliases, Set resolvedExpressions) { + boolean changed = false; + // check if the expression can be skipped or is not nullabe + if (skipExpression(exp) || exp.nullable() == Nullability.FALSE) { resolvedExpressions.add(exp); } else { - // expression might be nullable, go after it - if (exp.nullable() != Nullability.FALSE) { - for (Expression e : exp.references()) { - Expression resolved = aliases.resolve(e, e); - // found a root attribute, bail out - if (resolved instanceof Attribute a && resolved == e) { - resolvedExpressions.add(a); - } else { - // go further - doResolve(resolved, aliases, resolvedExpressions); - } + for (Expression e : exp.references()) { + Expression resolved = aliases.resolve(e, e); + // found a root attribute, bail out + if (resolved instanceof Attribute a && resolved == e) { + resolvedExpressions.add(a); + // don't mark things as change if the original expression hasn't been broken down + changed |= resolved != exp; + } else { + // go further + changed |= doResolve(resolved, aliases, resolvedExpressions); } - } else { - resolvedExpressions.add(exp); } } + return changed; } protected boolean skipExpression(Expression 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 087ceb525be0c..1cab7dd87195b 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 @@ -1771,6 +1771,15 @@ public void testPushDownFilterThroughAgg() throws Exception { assertEquals(expected, new PushDownAndCombineFilters().apply(fb)); } + public void testIsNotNullOnIsNullField() { + EsRelation relation = relation(); + var fieldA = getFieldAttribute("a"); + Expression inn = isNotNull(fieldA); + Filter f = new Filter(EMPTY, relation, inn); + + assertEquals(f, new OptimizerRules.InferIsNotNull().apply(f)); + } + public void testIsNotNullOnOperatorWithOneField() { EsRelation relation = relation(); var fieldA = getFieldAttribute("a"); From 4a6316d9a98e6ff2c84fb1ae9ea7688c800e759d Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Tue, 12 Dec 2023 18:08:22 -0800 Subject: [PATCH 5/5] Add test inside LocalLogicalPlanOptimizerTests --- .../LocalLogicalPlanOptimizerTests.java | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java index bc46189e13827..ac2426f485fcc 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java @@ -12,6 +12,7 @@ import org.elasticsearch.xpack.esql.analysis.Analyzer; import org.elasticsearch.xpack.esql.analysis.AnalyzerContext; import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry; +import org.elasticsearch.xpack.esql.expression.function.scalar.nulls.Coalesce; import org.elasticsearch.xpack.esql.parser.EsqlParser; import org.elasticsearch.xpack.esql.plan.logical.Eval; import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation; @@ -21,9 +22,11 @@ import org.elasticsearch.xpack.ql.expression.Expressions; import org.elasticsearch.xpack.ql.expression.Literal; import org.elasticsearch.xpack.ql.expression.ReferenceAttribute; +import org.elasticsearch.xpack.ql.expression.predicate.nulls.IsNotNull; import org.elasticsearch.xpack.ql.index.EsIndex; import org.elasticsearch.xpack.ql.index.IndexResolution; import org.elasticsearch.xpack.ql.plan.logical.EsRelation; +import org.elasticsearch.xpack.ql.plan.logical.Filter; import org.elasticsearch.xpack.ql.plan.logical.Limit; import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.ql.plan.logical.Project; @@ -35,6 +38,7 @@ import java.util.Map; import static org.elasticsearch.xpack.esql.EsqlTestUtils.L; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.TEST_SEARCH_STATS; import static org.elasticsearch.xpack.esql.EsqlTestUtils.TEST_VERIFIER; import static org.elasticsearch.xpack.esql.EsqlTestUtils.as; import static org.elasticsearch.xpack.esql.EsqlTestUtils.loadMapping; @@ -263,6 +267,38 @@ public void testMissingFieldInFilterNoProjection() { ); } + public void testIsNotNullOnCoalesce() { + var plan = localPlan(""" + from test + | where coalesce(emp_no, salary) is not null + """); + + var limit = as(plan, Limit.class); + var filter = as(limit.child(), Filter.class); + var inn = as(filter.condition(), IsNotNull.class); + var coalesce = as(inn.children().get(0), Coalesce.class); + assertThat(Expressions.names(coalesce.children()), contains("emp_no", "salary")); + var source = as(filter.child(), EsRelation.class); + } + + public void testIsNotNullOnExpression() { + var plan = localPlan(""" + from test + | eval x = emp_no + 1 + | where x is not null + """); + + var limit = as(plan, Limit.class); + var filter = as(limit.child(), Filter.class); + var inn = as(filter.condition(), IsNotNull.class); + assertThat(Expressions.names(inn.children()), contains("x")); + var eval = as(filter.child(), Eval.class); + filter = as(eval.child(), Filter.class); + inn = as(filter.condition(), IsNotNull.class); + assertThat(Expressions.names(inn.children()), contains("emp_no")); + var source = as(filter.child(), EsRelation.class); + } + private LocalRelation asEmptyRelation(Object o) { var empty = as(o, LocalRelation.class); assertThat(empty.supplier(), is(LocalSupplier.EMPTY)); @@ -285,6 +321,10 @@ private LogicalPlan localPlan(LogicalPlan plan, SearchStats searchStats) { return localPlan; } + private LogicalPlan localPlan(String query) { + return localPlan(plan(query), TEST_SEARCH_STATS); + } + @Override protected List filteredWarnings() { return withDefaultLimitWarning(super.filteredWarnings());