SQL: Introduce NotEquals node to simplify expressions (#35234)

Add NotEquals node in parser to simplify expressions so that <value1> != <value2> is no longer translated internally to NOT(<value1> = <value2>) Closes: #35210 Fixes: #35233
elastic · Nov 5, 2018 · a21316b · a21316b
1 parent 8bcc19d
commit a21316b
Show file tree

Hide file tree

Showing 17 changed files with 169 additions and 44 deletions.
diff --git a/.../elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java b/.../elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java
@@ -40,6 +40,7 @@
  * Acts as a registry of the various static methods used <b>internally</b> by the scalar functions
  * (to simplify the whitelist definition).
  */
+@SuppressWarnings("unused")
 public final class InternalSqlScriptUtils {
 
     private InternalSqlScriptUtils() {}
@@ -52,7 +53,7 @@ private InternalSqlScriptUtils() {}
     public static <T> Object docValue(Map<String, ScriptDocValues<T>> doc, String fieldName) {
         if (doc.containsKey(fieldName)) {
             ScriptDocValues<T> docValues = doc.get(fieldName);
-            if (docValues.size() > 0) {
+            if (!docValues.isEmpty()) {
                 return docValues.get(0);
             }
         }
@@ -83,6 +84,10 @@ public static Boolean eq(Object left, Object right) {
         return BinaryComparisonOperation.EQ.apply(left, right);
     }
 
+    public static Boolean neq(Object left, Object right) {
+        return BinaryComparisonOperation.NEQ.apply(left, right);
+    }
+
     public static Boolean lt(Object left, Object right) {
         return BinaryComparisonOperation.LT.apply(left, right);
     }

diff --git a/...csearch/xpack/sql/expression/predicate/operator/comparison/BinaryComparisonProcessor.java b/...csearch/xpack/sql/expression/predicate/operator/comparison/BinaryComparisonProcessor.java
@@ -19,6 +19,7 @@ public class BinaryComparisonProcessor extends FunctionalBinaryProcessor<Object,
     public enum BinaryComparisonOperation implements PredicateBiFunction<Object, Object, Boolean> {
 
         EQ(Comparisons::eq, "=="),
+        NEQ(Comparisons::neq, "!="),
         GT(Comparisons::gt, ">"),
         GTE(Comparisons::gte, ">="),
         LT(Comparisons::lt, "<"),
@@ -62,4 +63,4 @@ public BinaryComparisonProcessor(StreamInput in) throws IOException {
     public String getWriteableName() {
         return NAME;
     }
-}
+}
diff --git a/...ava/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/Comparisons.java b/...ava/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/Comparisons.java
@@ -19,6 +19,11 @@ public static Boolean eq(Object l, Object r) {
         return i == null ? null : i.intValue() == 0;
     }
 
+    static Boolean neq(Object l, Object r) {
+        Integer i = compare(l, r);
+        return i == null ? null : i.intValue() != 0;
+    }
+
     static Boolean lt(Object l, Object r) {
         Integer i = compare(l, r);
         return i == null ? null : i.intValue() < 0;
@@ -50,6 +55,9 @@ static Boolean in(Object l, Set<Object> r) {
      */
     @SuppressWarnings({ "rawtypes", "unchecked" })
     static Integer compare(Object l, Object r) {
+        if (l == null || r == null) {
+            return null;
+        }
         // typical number comparison
         if (l instanceof Number && r instanceof Number) {
             return compare((Number) l, (Number) r);

diff --git a/...ain/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/Equals.java b/...ain/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/Equals.java
@@ -6,11 +6,12 @@
 package org.elasticsearch.xpack.sql.expression.predicate.operator.comparison;
 
 import org.elasticsearch.xpack.sql.expression.Expression;
+import org.elasticsearch.xpack.sql.expression.predicate.BinaryOperator;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation;
 import org.elasticsearch.xpack.sql.tree.Location;
 import org.elasticsearch.xpack.sql.tree.NodeInfo;
 
-public class Equals extends BinaryComparison {
+public class Equals extends BinaryComparison implements BinaryOperator.Negateable {
 
     public Equals(Location location, Expression left, Expression right) {
         super(location, left, right, BinaryComparisonOperation.EQ);
@@ -30,4 +31,9 @@ protected Equals replaceChildren(Expression newLeft, Expression newRight) {
     public Equals swapLeftAndRight() {
         return new Equals(location(), right(), left());
     }
+
+    @Override
+    public BinaryOperator<?, ?, ?, ?> negate() {
+        return new NotEquals(location(), left(), right());
+    }
 }
diff --git a/.../java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/NotEquals.java b/.../java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/NotEquals.java
@@ -0,0 +1,39 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+package org.elasticsearch.xpack.sql.expression.predicate.operator.comparison;
+
+import org.elasticsearch.xpack.sql.expression.Expression;
+import org.elasticsearch.xpack.sql.expression.predicate.BinaryOperator;
+import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation;
+import org.elasticsearch.xpack.sql.tree.Location;
+import org.elasticsearch.xpack.sql.tree.NodeInfo;
+
+public class NotEquals extends BinaryComparison implements BinaryOperator.Negateable {
+
+    public NotEquals(Location location, Expression left, Expression right) {
+        super(location, left, right, BinaryComparisonOperation.NEQ);
+    }
+
+    @Override
+    protected NodeInfo<NotEquals> info() {
+        return NodeInfo.create(this, NotEquals::new, left(), right());
+    }
+
+    @Override
+    protected NotEquals replaceChildren(Expression newLeft, Expression newRight) {
+        return new NotEquals(location(), newLeft, newRight);
+    }
+
+    @Override
+    public NotEquals swapLeftAndRight() {
+        return new NotEquals(location(), right(), left());
+    }
+
+    @Override
+    public BinaryOperator<?, ?, ?, ?> negate() {
+        return new Equals(location(), left(), right());
+    }
+}
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
@@ -52,6 +52,7 @@
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.LessThan;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.LessThanOrEqual;
+import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.NotEquals;
 import org.elasticsearch.xpack.sql.plan.logical.Aggregate;
 import org.elasticsearch.xpack.sql.plan.logical.EsRelation;
 import org.elasticsearch.xpack.sql.plan.logical.Filter;
@@ -1313,7 +1314,7 @@ private Expression simplify(BinaryComparison bc) {
             }
 
             // false for equality
-            if (bc instanceof GreaterThan || bc instanceof LessThan) {
+            if (bc instanceof NotEquals || bc instanceof GreaterThan || bc instanceof LessThan) {
                 if (!l.nullable() && !r.nullable() && l.semanticEquals(r)) {
                     return FALSE;
                 }

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java
@@ -44,6 +44,7 @@
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.GreaterThanOrEqual;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.LessThan;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.LessThanOrEqual;
+import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.NotEquals;
 import org.elasticsearch.xpack.sql.expression.predicate.regex.Like;
 import org.elasticsearch.xpack.sql.expression.predicate.regex.LikePattern;
 import org.elasticsearch.xpack.sql.expression.predicate.regex.RLike;
@@ -165,7 +166,7 @@ public Expression visitComparison(ComparisonContext ctx) {
             case SqlBaseParser.EQ:
                 return new Equals(loc, left, right);
             case SqlBaseParser.NEQ:
-                return new Not(loc, new Equals(loc, left, right));
+                return new NotEquals(loc, left, right);
             case SqlBaseParser.LT:
                 return new LessThan(loc, left, right);
             case SqlBaseParser.LTE:

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java
@@ -31,7 +31,6 @@
 import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFunction;
 import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeHistogramFunction;
 import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate;
-import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In;
 import org.elasticsearch.xpack.sql.expression.predicate.IsNotNull;
 import org.elasticsearch.xpack.sql.expression.predicate.Range;
 import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MatchQueryPredicate;
@@ -44,8 +43,10 @@
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.Equals;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.GreaterThan;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.GreaterThanOrEqual;
+import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.LessThan;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.LessThanOrEqual;
+import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.NotEquals;
 import org.elasticsearch.xpack.sql.expression.predicate.regex.Like;
 import org.elasticsearch.xpack.sql.expression.predicate.regex.LikePattern;
 import org.elasticsearch.xpack.sql.expression.predicate.regex.RLike;
@@ -536,16 +537,15 @@ protected QueryTranslation asQuery(BinaryComparison bc, boolean onAggs) {
                 //
                 // Agg context means HAVING -> PipelineAggs
                 //
-                ScriptTemplate script = bc.asScript();
                 if (onAggs) {
-                    aggFilter = new AggFilter(at.id().toString(), script);
+                    aggFilter = new AggFilter(at.id().toString(), bc.asScript());
                 }
                 else {
                     // query directly on the field
                     if (at instanceof FieldAttribute) {
                         query = wrapIfNested(translateQuery(bc), ne);
                     } else {
-                        query = new ScriptQuery(at.location(), script);
+                        query = new ScriptQuery(at.location(), bc.asScript());
                     }
                 }
                 return new QueryTranslation(query, aggFilter);
@@ -576,7 +576,7 @@ private static Query translateQuery(BinaryComparison bc) {
             if (bc instanceof LessThanOrEqual) {
                 return new RangeQuery(loc, name, null, false, value, true, format);
             }
-            if (bc instanceof Equals) {
+            if (bc instanceof Equals || bc instanceof NotEquals) {
                 if (bc.left() instanceof FieldAttribute) {
                     FieldAttribute fa = (FieldAttribute) bc.left();
                     // equality should always be against an exact match
@@ -585,7 +585,11 @@ private static Query translateQuery(BinaryComparison bc) {
                         name = fa.exactAttribute().name();
                     }
                 }
-                return new TermQuery(loc, name, value);
+                Query query = new TermQuery(loc, name, value);
+                if (bc instanceof NotEquals) {
+                    query = new NotQuery(loc, query);
+                }
+                return query;
             }
 
             throw new SqlIllegalArgumentException("Don't know how to translate binary comparison [{}] in [{}]", bc.right().nodeString(),
@@ -655,11 +659,10 @@ protected QueryTranslation asQuery(Range r, boolean onAggs) {
                 //
                 // Agg context means HAVING -> PipelineAggs
                 //
-                ScriptTemplate script = r.asScript();
                 Attribute at = ((NamedExpression) e).toAttribute();
 
                 if (onAggs) {
-                    aggFilter = new AggFilter(at.id().toString(), script);
+                    aggFilter = new AggFilter(at.id().toString(), r.asScript());
                 } else {
                     // typical range; no scripting involved
                     if (at instanceof FieldAttribute) {
@@ -669,7 +672,7 @@ protected QueryTranslation asQuery(Range r, boolean onAggs) {
                     }
                     // scripted query
                     else {
-                        query = new ScriptQuery(at.location(), script);
+                        query = new ScriptQuery(at.location(), r.asScript());
                     }
                 }
                 return new QueryTranslation(query, aggFilter);

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/NotQuery.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/NotQuery.java
@@ -9,10 +9,10 @@
 import org.elasticsearch.search.sort.NestedSortBuilder;
 import org.elasticsearch.xpack.sql.tree.Location;
 
-import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
-
 import java.util.Objects;
 
+import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
+
 public class NotQuery extends Query {
     private final Query child;
 

diff --git a/x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt b/x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt
@@ -20,6 +20,7 @@ class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalS
 # Comparison
 #
   Boolean eq(Object, Object)
+  Boolean neq(Object, Object)
   Boolean lt(Object, Object)
   Boolean lte(Object, Object)
   Boolean gt(Object, Object)

diff --git a/...ch/xpack/sql/expression/predicate/operator/comparison/BinaryComparisonProcessorTests.java b/...ch/xpack/sql/expression/predicate/operator/comparison/BinaryComparisonProcessorTests.java
@@ -11,12 +11,6 @@
 import org.elasticsearch.xpack.sql.expression.Literal;
 import org.elasticsearch.xpack.sql.expression.function.scalar.Processors;
 import org.elasticsearch.xpack.sql.expression.gen.processor.ConstantProcessor;
-import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
-import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.Equals;
-import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.GreaterThan;
-import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.GreaterThanOrEqual;
-import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.LessThan;
-import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.LessThanOrEqual;
 
 import static org.elasticsearch.xpack.sql.tree.Location.EMPTY;
 
@@ -48,6 +42,11 @@ public void testEq() {
         assertEquals(false, new Equals(EMPTY, l(3), l(4)).makePipe().asProcessor().process(null));
     }
 
+    public void testNEq() {
+        assertEquals(false, new NotEquals(EMPTY, l(4), l(4)).makePipe().asProcessor().process(null));
+        assertEquals(true, new NotEquals(EMPTY, l(3), l(4)).makePipe().asProcessor().process(null));
+    }
+
     public void testGt() {
         assertEquals(true, new GreaterThan(EMPTY, l(4), l(3)).makePipe().asProcessor().process(null));
         assertEquals(false, new GreaterThan(EMPTY, l(3), l(4)).makePipe().asProcessor().process(null));
@@ -73,14 +72,15 @@ public void testLte() {
     }
 
     public void testHandleNull() {
-        assertNull(new Equals(EMPTY, l(null), l(3)).makePipe().asProcessor().process(null));
-        assertNull(new GreaterThan(EMPTY, l(null), l(3)).makePipe().asProcessor().process(null));
-        assertNull(new GreaterThanOrEqual(EMPTY, l(null), l(3)).makePipe().asProcessor().process(null));
-        assertNull(new LessThan(EMPTY, l(null), l(3)).makePipe().asProcessor().process(null));
-        assertNull(new LessThanOrEqual(EMPTY, l(null), l(3)).makePipe().asProcessor().process(null));
+        assertNull(new Equals(EMPTY, Literal.NULL, l(3)).makePipe().asProcessor().process(null));
+        assertNull(new NotEquals(EMPTY, Literal.NULL, l(3)).makePipe().asProcessor().process(null));
+        assertNull(new GreaterThan(EMPTY, Literal.NULL, l(3)).makePipe().asProcessor().process(null));
+        assertNull(new GreaterThanOrEqual(EMPTY, Literal.NULL, l(3)).makePipe().asProcessor().process(null));
+        assertNull(new LessThan(EMPTY, Literal.NULL, l(3)).makePipe().asProcessor().process(null));
+        assertNull(new LessThanOrEqual(EMPTY, Literal.NULL, l(3)).makePipe().asProcessor().process(null));
     }
 
     private static Literal l(Object value) {
         return Literal.of(EMPTY, value);
     }
-}
+}
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
@@ -35,7 +35,6 @@
 import org.elasticsearch.xpack.sql.expression.function.scalar.string.Ascii;
 import org.elasticsearch.xpack.sql.expression.function.scalar.string.Repeat;
 import org.elasticsearch.xpack.sql.expression.predicate.BinaryOperator;
-import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In;
 import org.elasticsearch.xpack.sql.expression.predicate.IsNotNull;
 import org.elasticsearch.xpack.sql.expression.predicate.Range;
 import org.elasticsearch.xpack.sql.expression.predicate.logical.And;
@@ -49,8 +48,10 @@
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.Equals;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.GreaterThan;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.GreaterThanOrEqual;
+import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.LessThan;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.LessThanOrEqual;
+import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.NotEquals;
 import org.elasticsearch.xpack.sql.expression.predicate.regex.Like;
 import org.elasticsearch.xpack.sql.expression.predicate.regex.LikePattern;
 import org.elasticsearch.xpack.sql.expression.predicate.regex.RLike;
@@ -265,6 +266,7 @@ public void testConstantFoldingBinaryComparison() {
         assertEquals(Literal.FALSE, new ConstantFolding().rule(new GreaterThan(EMPTY, TWO, THREE)).canonical());
         assertEquals(Literal.FALSE, new ConstantFolding().rule(new GreaterThanOrEqual(EMPTY, TWO, THREE)).canonical());
         assertEquals(Literal.FALSE, new ConstantFolding().rule(new Equals(EMPTY, TWO, THREE)).canonical());
+        assertEquals(Literal.TRUE, new ConstantFolding().rule(new NotEquals(EMPTY, TWO, THREE)).canonical());
         assertEquals(Literal.TRUE, new ConstantFolding().rule(new LessThanOrEqual(EMPTY, TWO, THREE)).canonical());
         assertEquals(Literal.TRUE, new ConstantFolding().rule(new LessThan(EMPTY, TWO, THREE)).canonical());
     }
@@ -406,11 +408,12 @@ public void testGenericNullableExpression() {
 
     private void assertNullLiteral(Expression expression) {
         assertEquals(Literal.class, expression.getClass());
-        assertNull(((Literal) expression).fold());
+        assertNull(expression.fold());
     }
 
     public void testBinaryComparisonSimplification() {
         assertEquals(Literal.TRUE, new BinaryComparisonSimplification().rule(new Equals(EMPTY, FIVE, FIVE)));
+        assertEquals(Literal.FALSE, new BinaryComparisonSimplification().rule(new NotEquals(EMPTY, FIVE, FIVE)));
         assertEquals(Literal.TRUE, new BinaryComparisonSimplification().rule(new GreaterThanOrEqual(EMPTY, FIVE, FIVE)));
         assertEquals(Literal.TRUE, new BinaryComparisonSimplification().rule(new LessThanOrEqual(EMPTY, FIVE, FIVE)));
 

diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java
@@ -14,6 +14,8 @@
 import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Mul;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Neg;
 import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Sub;
+import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.Equals;
+import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.NotEquals;
 import org.elasticsearch.xpack.sql.type.DataType;
 
 import static org.hamcrest.core.StringStartsWith.startsWith;
@@ -158,6 +160,22 @@ public void testComplexArithmetic() {
         assertEquals("2", ((Literal) sub2.children().get(1)).name());
     }
 
+    public void testEquals() {
+        Expression expr = parser.createExpression("a = 10");
+        assertEquals(Equals.class, expr.getClass());
+        Equals eq = (Equals) expr;
+        assertEquals("(a) == 10", eq.name());
+        assertEquals(2, eq.children().size());
+    }
+
+    public void testNotEquals() {
+        Expression expr = parser.createExpression("a != 10");
+        assertEquals(NotEquals.class, expr.getClass());
+        NotEquals neq = (NotEquals) expr;
+        assertEquals("(a) != 10", neq.name());
+        assertEquals(2, neq.children().size());
+    }
+
     public void testCastWithUnquotedDataType() {
         Expression expr = parser.createExpression("CAST(10*2 AS long)");
         assertEquals(Cast.class, expr.getClass());

diff --git a/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java
@@ -34,7 +34,7 @@
 import java.util.TimeZone;
 
 import static org.hamcrest.Matchers.endsWith;
-import static org.hamcrest.core.StringStartsWith.startsWith;
+import static org.hamcrest.Matchers.startsWith;
 
 public class QueryTranslatorTests extends ESTestCase {