[7.12] EQL: Optimize redundant toString (#71070) (#71269)

Replace toString("string") with "string" Refactor the underlying rule to reuse code across QL Fix #70671
elastic · Apr 3, 2021 · f174af8 · f174af8
1 parent 541d282
commit f174af8
Show file tree

Hide file tree

Showing 6 changed files with 77 additions and 16 deletions.
diff --git a/...src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/ToString.java b/...src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/ToString.java
@@ -36,9 +36,9 @@ public class ToString extends ScalarFunction {
 
     private final Expression value;
 
-    public ToString(Source source, Expression src) {
-        super(source, Collections.singletonList(src));
-        this.value = src;
+    public ToString(Source source, Expression value) {
+        super(source, Collections.singletonList(value));
+        this.value = value;
     }
 
     @Override
@@ -50,6 +50,10 @@ protected TypeResolution resolveType() {
         return isExact(value, sourceText(), ParamOrdinal.DEFAULT);
     }
 
+    public Expression value() {
+        return value;
+    }
+
     @Override
     protected Pipe makePipe() {
         return new ToStringFunctionPipe(source(), this, Expressions.pipe(value));

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
@@ -8,6 +8,7 @@
 package org.elasticsearch.xpack.eql.optimizer;
 
 import org.elasticsearch.xpack.eql.EqlIllegalArgumentException;
+import org.elasticsearch.xpack.eql.expression.function.scalar.string.ToString;
 import org.elasticsearch.xpack.eql.expression.predicate.operator.comparison.InsensitiveBinaryComparison;
 import org.elasticsearch.xpack.eql.expression.predicate.operator.comparison.InsensitiveEquals;
 import org.elasticsearch.xpack.eql.expression.predicate.operator.comparison.InsensitiveWildcardEquals;
@@ -36,6 +37,7 @@
 import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NotEquals;
 import org.elasticsearch.xpack.ql.expression.predicate.regex.Like;
 import org.elasticsearch.xpack.ql.expression.predicate.regex.RegexMatch;
+import org.elasticsearch.xpack.ql.optimizer.OptimizerRules;
 import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanFunctionEqualsElimination;
 import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight;
 import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification;
@@ -94,6 +96,7 @@ protected Iterable<RuleExecutor<LogicalPlan>.Batch> batches() {
                 // prune/elimination
                 new PruneFilters(),
                 new PruneLiteralsInOrderBy(),
+                new PruneCast(),
                 new CombineLimits());
 
         Batch constraints = new Batch("Infer constraints", Limiter.ONCE,
@@ -229,6 +232,18 @@ protected LogicalPlan rule(UnaryPlan plan) {
         }
     }
 
+    static class PruneCast extends OptimizerRules.PruneCast<ToString> {
+
+        PruneCast() {
+            super(ToString.class);
+        }
+
+        @Override
+        protected Expression maybePruneCast(ToString cast) {
+            return cast.dataType().equals(cast.value().dataType()) ? cast.value() : cast;
+        }
+    }
+
     static class SkipQueryOnLimitZero extends org.elasticsearch.xpack.ql.optimizer.OptimizerRules.SkipQueryOnLimitZero {
 
         @Override

diff --git a/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java b/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java
@@ -14,13 +14,15 @@
 import org.elasticsearch.xpack.eql.analysis.PreAnalyzer;
 import org.elasticsearch.xpack.eql.analysis.Verifier;
 import org.elasticsearch.xpack.eql.expression.function.EqlFunctionRegistry;
+import org.elasticsearch.xpack.eql.expression.function.scalar.string.ToString;
 import org.elasticsearch.xpack.eql.parser.EqlParser;
 import org.elasticsearch.xpack.eql.plan.logical.KeyedFilter;
 import org.elasticsearch.xpack.eql.plan.logical.LimitWithOffset;
 import org.elasticsearch.xpack.eql.plan.logical.Sequence;
 import org.elasticsearch.xpack.eql.plan.logical.Tail;
 import org.elasticsearch.xpack.eql.plan.physical.LocalRelation;
 import org.elasticsearch.xpack.eql.stats.Metrics;
+import org.elasticsearch.xpack.ql.TestUtils;
 import org.elasticsearch.xpack.ql.expression.Attribute;
 import org.elasticsearch.xpack.ql.expression.EmptyAttribute;
 import org.elasticsearch.xpack.ql.expression.Expression;
@@ -48,6 +50,7 @@
 import org.elasticsearch.xpack.ql.plan.logical.Project;
 import org.elasticsearch.xpack.ql.plan.logical.UnaryPlan;
 import org.elasticsearch.xpack.ql.plan.logical.UnresolvedRelation;
+import org.elasticsearch.xpack.ql.type.DataTypes;
 import org.elasticsearch.xpack.ql.type.EsField;
 import org.elasticsearch.xpack.ql.type.TypesTests;
 
@@ -689,6 +692,18 @@ public void testReduceBinaryComparisons() {
         assertEquals(6, ((Literal) gte.right()).value());
     }
 
+    public void testReplaceCastOnField() {
+        Attribute a = TestUtils.fieldAttribute("string", DataTypes.KEYWORD);
+        ToString ts = new ToString(EMPTY, a);
+        assertSame(a, new Optimizer.PruneCast().maybePruneCast(ts));
+    }
+
+    public void testReplaceCastOnLiteral() {
+        Literal l = new Literal(EMPTY, "string", DataTypes.KEYWORD);
+        ToString ts = new ToString(EMPTY, l);
+        assertSame(l, new Optimizer.PruneCast().maybePruneCast(ts));
+    }
+
     private static Attribute timestamp() {
         return new FieldAttribute(EMPTY, "test", new EsField("field", INTEGER, emptyMap(), true));
     }

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
@@ -1443,6 +1443,28 @@ protected LogicalPlan rule(OrderBy ob) {
         }
     }
 
+    // NB: it is important to start replacing casts from the bottom to properly replace aliases
+    public abstract static class PruneCast<C extends Expression> extends Rule<LogicalPlan, LogicalPlan> {
+
+        private final Class<C> castType;
+
+        public PruneCast(Class<C> castType) {
+            this.castType = castType;
+        }
+
+        @Override
+        public final LogicalPlan apply(LogicalPlan plan) {
+            return rule(plan);
+        }
+
+        @Override
+        protected final LogicalPlan rule(LogicalPlan plan) {
+            // eliminate redundant casts
+            return plan.transformExpressionsUp(castType, this::maybePruneCast);
+        }
+
+        protected abstract Expression maybePruneCast(C cast);
+    }
 
     public abstract static class SkipQueryOnLimitZero extends OptimizerRule<Limit> {
         @Override

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java
@@ -35,6 +35,7 @@
 import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.LessThanOrEqual;
 import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NotEquals;
 import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NullEquals;
+import org.elasticsearch.xpack.ql.optimizer.OptimizerRules;
 import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight;
 import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification;
 import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineBinaryComparisons;
@@ -494,23 +495,15 @@ protected LogicalPlan rule(Limit limit) {
         }
     }
 
-    // NB: it is important to start replacing casts from the bottom to properly replace aliases
-    static class PruneCast extends Rule<LogicalPlan, LogicalPlan> {
+    static class PruneCast extends OptimizerRules.PruneCast<Cast> {
 
-        @Override
-        public LogicalPlan apply(LogicalPlan plan) {
-            return rule(plan);
+        PruneCast() {
+            super(Cast.class);
         }
 
         @Override
-        protected LogicalPlan rule(LogicalPlan plan) {
-            // eliminate redundant casts
-            return plan.transformExpressionsUp(Cast.class, c -> {
-                if (c.from() == c.to()) {
-                    return c.field();
-                }
-                return c;
-            });
+        protected Expression maybePruneCast(Cast cast) {
+            return cast.from() == cast.to() ? cast.field() : cast;
         }
     }
 

diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java
@@ -1044,6 +1044,18 @@ public void testSumIsReplacedWithStats() {
         assertEquals(sum, ((InnerAggregate) alias.child()).inner());
     }
 
+    public void testReplaceCast() {
+        FieldAttribute a = getFieldAttribute("a");
+        Cast c = new Cast(EMPTY, a, a.dataType());
+        assertSame(a, new Optimizer.PruneCast().maybePruneCast(c));
+    }
+
+    public void testReplaceCastOnLiteral() {
+        Literal literal = literal("string");
+        Cast c = new Cast(EMPTY, literal, literal.dataType());
+        assertSame(literal, new Optimizer.PruneCast().maybePruneCast(c));
+    }
+
     /**
      * Once the root cause of https://github.com/elastic/elasticsearch/issues/45251 is fixed in the <code>sum</code> ES aggregation
      * (can differentiate between <code>SUM(all zeroes)</code> and <code>SUM(all nulls)</code>),