[ESQL] Binary Comparison Serialization (elastic#107921)

Prior to this PR, serializing a binary comparison in ES|QL depended on the enum BinaryComparisonProcessor.BinaryComparisonOperator from the QL binary comparison code. That put some distance between the ESQL classes and their serialization logic, while also limiting our ability to make adjustments to that logic (since doing so would have ramifications for SQL and EQL) This PR introduces a new ESQL specific enum for binary comparisons, which has a Writer and a Reader built in, and which implements the standard Writable interface. This enum is constructed in such a way as to be wire-compatible with the existing enum, thus not requiring a transport version change (although any future changes to this probably will require a transport version change). A side effect of this change is removing Null Equals from ESQL serialization. We never actually implemented Null Equals, and the existing class is a stub. I infer that it was only created to allow use of the QL BinaryComparisonOperator enum, which specifies a Null Equals. I did not include it in the ESQL specific enum I just added, and as such removed it from places that reference that enum. There is also a "shim" mapping from the new ESQL specific enum to the general QL enum. This is necessary for passing up to the parent BinaryOperation class. Changing the argument for that to use an interface like ArithmeticOperation does would require some non-trivial changes to how QL does serialization, which would dramatically increase the surface area of this PR. Medium term, I would like to change EsqlBinaryComparison to inherit directly from BinaryOperator, which will remove the need for that shim. Unfortunately, doing so proved non-trivial, and so I'm saving that for follow up work. Follow up work: - Remove remaining references to Null Equals, and the ESQL Null Equals class. - Move PlanNamedTypes.writeBinComparison and PlanNamedTypes.readBinComparison into EsqlBinaryComparison, and make EsqlBinaryComparison Writable. This will finish putting the serialization logic next to the object being serialized, for binary comparisons. - Remove the "shim" by changing EsqlBinaryComparison to inherit directly from BinaryOperation
lcawl · Apr 26, 2024 · 4664ced · 4664ced
1 parent 22aad7b
commit 4664ced
Show file tree

Hide file tree

Showing 10 changed files with 191 additions and 87 deletions.
diff --git a/...ain/java/org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/Equals.java b/...ain/java/org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/Equals.java
@@ -12,8 +12,6 @@
 import org.elasticsearch.xpack.esql.type.EsqlDataTypes;
 import org.elasticsearch.xpack.ql.expression.Expression;
 import org.elasticsearch.xpack.ql.expression.predicate.Negatable;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
 import org.elasticsearch.xpack.ql.tree.NodeInfo;
 import org.elasticsearch.xpack.ql.tree.Source;
 import org.elasticsearch.xpack.ql.type.DataType;
@@ -22,7 +20,7 @@
 import java.time.ZoneId;
 import java.util.Map;
 
-public class Equals extends EsqlBinaryComparison implements Negatable<BinaryComparison> {
+public class Equals extends EsqlBinaryComparison implements Negatable<EsqlBinaryComparison> {
     private static final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap = Map.ofEntries(
         Map.entry(DataTypes.BOOLEAN, EqualsBoolsEvaluator.Factory::new),
         Map.entry(DataTypes.INTEGER, EqualsIntsEvaluator.Factory::new),
@@ -41,11 +39,11 @@ public class Equals extends EsqlBinaryComparison implements Negatable<BinaryComp
     );
 
     public Equals(Source source, Expression left, Expression right) {
-        super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.EQ, evaluatorMap);
+        super(source, left, right, BinaryComparisonOperation.EQ, evaluatorMap);
     }
 
     public Equals(Source source, Expression left, Expression right, ZoneId zoneId) {
-        super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.EQ, zoneId, evaluatorMap);
+        super(source, left, right, BinaryComparisonOperation.EQ, zoneId, evaluatorMap);
     }
 
     @Override
@@ -64,12 +62,12 @@ public Equals swapLeftAndRight() {
     }
 
     @Override
-    public BinaryComparison reverse() {
+    public EsqlBinaryComparison reverse() {
         return this;
     }
 
     @Override
-    public BinaryComparison negate() {
+    public EsqlBinaryComparison negate() {
         return new NotEquals(source(), left(), right(), zoneId());
     }
 

diff --git a/...lasticsearch/xpack/esql/evaluator/predicate/operator/comparison/EsqlBinaryComparison.java b/...lasticsearch/xpack/esql/evaluator/predicate/operator/comparison/EsqlBinaryComparison.java
@@ -7,6 +7,9 @@
 
 package org.elasticsearch.xpack.esql.evaluator.predicate.operator.comparison;
 
+import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.io.stream.Writeable;
 import org.elasticsearch.compute.operator.EvalOperator;
 import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
 import org.elasticsearch.xpack.esql.evaluator.mapper.EvaluatorMapper;
@@ -21,6 +24,7 @@
 import org.elasticsearch.xpack.ql.type.DataType;
 import org.elasticsearch.xpack.ql.type.DataTypes;
 
+import java.io.IOException;
 import java.time.ZoneId;
 import java.util.Map;
 import java.util.function.Function;
@@ -32,14 +36,66 @@ public abstract class EsqlBinaryComparison extends BinaryComparison implements E
 
     private final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap;
 
+    private final BinaryComparisonOperation functionType;
+
+    @FunctionalInterface
+    public interface BinaryOperatorConstructor {
+        EsqlBinaryComparison apply(Source source, Expression lhs, Expression rhs);
+    }
+
+    public enum BinaryComparisonOperation implements Writeable {
+
+        EQ(0, "==", BinaryComparisonProcessor.BinaryComparisonOperation.EQ, Equals::new),
+        // id 1 reserved for NullEquals
+        NEQ(2, "!=", BinaryComparisonProcessor.BinaryComparisonOperation.NEQ, NotEquals::new),
+        GT(3, ">", BinaryComparisonProcessor.BinaryComparisonOperation.GT, GreaterThan::new),
+        GTE(4, ">=", BinaryComparisonProcessor.BinaryComparisonOperation.GTE, GreaterThanOrEqual::new),
+        LT(5, "<", BinaryComparisonProcessor.BinaryComparisonOperation.LT, LessThan::new),
+        LTE(6, "<=", BinaryComparisonProcessor.BinaryComparisonOperation.LTE, LessThanOrEqual::new);
+
+        private final int id;
+        private final String symbol;
+        // Temporary mapping to the old enum, to satisfy the superclass constructor signature.
+        private final BinaryComparisonProcessor.BinaryComparisonOperation shim;
+        private final BinaryOperatorConstructor constructor;
+
+        BinaryComparisonOperation(
+            int id,
+            String symbol,
+            BinaryComparisonProcessor.BinaryComparisonOperation shim,
+            BinaryOperatorConstructor constructor
+        ) {
+            this.id = id;
+            this.symbol = symbol;
+            this.shim = shim;
+            this.constructor = constructor;
+        }
+
+        @Override
+        public void writeTo(StreamOutput out) throws IOException {
+            out.writeVInt(id);
+        }
+
+        public static BinaryComparisonOperation readFromStream(StreamInput in) throws IOException {
+            int id = in.readVInt();
+            for (BinaryComparisonOperation op : values()) {
+                if (op.id == id) {
+                    return op;
+                }
+            }
+            throw new IOException("No BinaryComparisonOperation found for id [" + id + "]");
+        }
+
+        public EsqlBinaryComparison buildNewInstance(Source source, Expression lhs, Expression rhs) {
+            return constructor.apply(source, lhs, rhs);
+        }
+    }
+
     protected EsqlBinaryComparison(
         Source source,
         Expression left,
         Expression right,
-        /* TODO: BinaryComparisonOperator is an enum with a bunch of functionality we don't really want. We should extract an interface and
-                 create a symbol only version like we did for BinaryArithmeticOperation. Ideally, they could be the same class.
-         */
-        BinaryComparisonProcessor.BinaryComparisonOperation operation,
+        BinaryComparisonOperation operation,
         Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap
     ) {
         this(source, left, right, operation, null, evaluatorMap);
@@ -49,13 +105,18 @@ protected EsqlBinaryComparison(
         Source source,
         Expression left,
         Expression right,
-        BinaryComparisonProcessor.BinaryComparisonOperation operation,
+        BinaryComparisonOperation operation,
         // TODO: We are definitely not doing the right thing with this zoneId
         ZoneId zoneId,
         Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap
     ) {
-        super(source, left, right, operation, zoneId);
+        super(source, left, right, operation.shim, zoneId);
         this.evaluatorMap = evaluatorMap;
+        this.functionType = operation;
+    }
+
+    public BinaryComparisonOperation getFunctionType() {
+        return functionType;
     }
 
     @Override

diff --git a/...ava/org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/GreaterThan.java b/...ava/org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/GreaterThan.java
@@ -11,8 +11,6 @@
 import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
 import org.elasticsearch.xpack.ql.expression.Expression;
 import org.elasticsearch.xpack.ql.expression.predicate.Negatable;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
 import org.elasticsearch.xpack.ql.tree.NodeInfo;
 import org.elasticsearch.xpack.ql.tree.Source;
 import org.elasticsearch.xpack.ql.type.DataType;
@@ -21,7 +19,7 @@
 import java.time.ZoneId;
 import java.util.Map;
 
-public class GreaterThan extends EsqlBinaryComparison implements Negatable<BinaryComparison> {
+public class GreaterThan extends EsqlBinaryComparison implements Negatable<EsqlBinaryComparison> {
     private static final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap = Map.ofEntries(
         Map.entry(DataTypes.INTEGER, GreaterThanIntsEvaluator.Factory::new),
         Map.entry(DataTypes.DOUBLE, GreaterThanDoublesEvaluator.Factory::new),
@@ -35,11 +33,11 @@ public class GreaterThan extends EsqlBinaryComparison implements Negatable<Binar
     );
 
     public GreaterThan(Source source, Expression left, Expression right) {
-        super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.GT, evaluatorMap);
+        super(source, left, right, BinaryComparisonOperation.GT, evaluatorMap);
     }
 
     public GreaterThan(Source source, Expression left, Expression right, ZoneId zoneId) {
-        super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.GT, zoneId, evaluatorMap);
+        super(source, left, right, BinaryComparisonOperation.GT, zoneId, evaluatorMap);
     }
 
     @Override
@@ -63,7 +61,7 @@ public LessThanOrEqual negate() {
     }
 
     @Override
-    public BinaryComparison reverse() {
+    public EsqlBinaryComparison reverse() {
         return new LessThan(source(), left(), right(), zoneId());
     }
 

diff --git a/.../elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/GreaterThanOrEqual.java b/.../elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/GreaterThanOrEqual.java
@@ -11,8 +11,6 @@
 import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
 import org.elasticsearch.xpack.ql.expression.Expression;
 import org.elasticsearch.xpack.ql.expression.predicate.Negatable;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
 import org.elasticsearch.xpack.ql.tree.NodeInfo;
 import org.elasticsearch.xpack.ql.tree.Source;
 import org.elasticsearch.xpack.ql.type.DataType;
@@ -21,7 +19,7 @@
 import java.time.ZoneId;
 import java.util.Map;
 
-public class GreaterThanOrEqual extends EsqlBinaryComparison implements Negatable<BinaryComparison> {
+public class GreaterThanOrEqual extends EsqlBinaryComparison implements Negatable<EsqlBinaryComparison> {
     private static final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap = Map.ofEntries(
         Map.entry(DataTypes.INTEGER, GreaterThanOrEqualIntsEvaluator.Factory::new),
         Map.entry(DataTypes.DOUBLE, GreaterThanOrEqualDoublesEvaluator.Factory::new),
@@ -35,11 +33,11 @@ public class GreaterThanOrEqual extends EsqlBinaryComparison implements Negatabl
     );
 
     public GreaterThanOrEqual(Source source, Expression left, Expression right) {
-        super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.GTE, evaluatorMap);
+        super(source, left, right, BinaryComparisonOperation.GTE, evaluatorMap);
     }
 
     public GreaterThanOrEqual(Source source, Expression left, Expression right, ZoneId zoneId) {
-        super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.GTE, zoneId, evaluatorMap);
+        super(source, left, right, BinaryComparisonOperation.GTE, zoneId, evaluatorMap);
     }
 
     @Override
@@ -63,7 +61,7 @@ public LessThan negate() {
     }
 
     @Override
-    public BinaryComparison reverse() {
+    public EsqlBinaryComparison reverse() {
         return new LessThanOrEqual(source(), left(), right(), zoneId());
     }
 

diff --git a/...n/java/org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/LessThan.java b/...n/java/org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/LessThan.java
@@ -11,8 +11,6 @@
 import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
 import org.elasticsearch.xpack.ql.expression.Expression;
 import org.elasticsearch.xpack.ql.expression.predicate.Negatable;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
 import org.elasticsearch.xpack.ql.tree.NodeInfo;
 import org.elasticsearch.xpack.ql.tree.Source;
 import org.elasticsearch.xpack.ql.type.DataType;
@@ -21,7 +19,7 @@
 import java.time.ZoneId;
 import java.util.Map;
 
-public class LessThan extends EsqlBinaryComparison implements Negatable<BinaryComparison> {
+public class LessThan extends EsqlBinaryComparison implements Negatable<EsqlBinaryComparison> {
 
     private static final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap = Map.ofEntries(
         Map.entry(DataTypes.INTEGER, LessThanIntsEvaluator.Factory::new),
@@ -35,8 +33,12 @@ public class LessThan extends EsqlBinaryComparison implements Negatable<BinaryCo
         Map.entry(DataTypes.IP, LessThanKeywordsEvaluator.Factory::new)
     );
 
+    public LessThan(Source source, Expression left, Expression right) {
+        this(source, left, right, null);
+    }
+
     public LessThan(Source source, Expression left, Expression right, ZoneId zoneId) {
-        super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.LT, zoneId, evaluatorMap);
+        super(source, left, right, BinaryComparisonOperation.LT, zoneId, evaluatorMap);
     }
 
     @Override
@@ -60,7 +62,7 @@ public GreaterThanOrEqual negate() {
     }
 
     @Override
-    public BinaryComparison reverse() {
+    public EsqlBinaryComparison reverse() {
         return new GreaterThan(source(), left(), right(), zoneId());
     }
 

diff --git a/...org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/LessThanOrEqual.java b/...org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/LessThanOrEqual.java
@@ -11,8 +11,6 @@
 import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
 import org.elasticsearch.xpack.ql.expression.Expression;
 import org.elasticsearch.xpack.ql.expression.predicate.Negatable;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
 import org.elasticsearch.xpack.ql.tree.NodeInfo;
 import org.elasticsearch.xpack.ql.tree.Source;
 import org.elasticsearch.xpack.ql.type.DataType;
@@ -21,7 +19,7 @@
 import java.time.ZoneId;
 import java.util.Map;
 
-public class LessThanOrEqual extends EsqlBinaryComparison implements Negatable<BinaryComparison> {
+public class LessThanOrEqual extends EsqlBinaryComparison implements Negatable<EsqlBinaryComparison> {
     private static final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap = Map.ofEntries(
         Map.entry(DataTypes.INTEGER, LessThanOrEqualIntsEvaluator.Factory::new),
         Map.entry(DataTypes.DOUBLE, LessThanOrEqualDoublesEvaluator.Factory::new),
@@ -34,8 +32,12 @@ public class LessThanOrEqual extends EsqlBinaryComparison implements Negatable<B
         Map.entry(DataTypes.IP, LessThanOrEqualKeywordsEvaluator.Factory::new)
     );
 
+    public LessThanOrEqual(Source source, Expression left, Expression right) {
+        this(source, left, right, null);
+    }
+
     public LessThanOrEqual(Source source, Expression left, Expression right, ZoneId zoneId) {
-        super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.LTE, zoneId, evaluatorMap);
+        super(source, left, right, BinaryComparisonOperation.LTE, zoneId, evaluatorMap);
     }
 
     @Override
@@ -59,7 +61,7 @@ public GreaterThan negate() {
     }
 
     @Override
-    public BinaryComparison reverse() {
+    public EsqlBinaryComparison reverse() {
         return new GreaterThanOrEqual(source(), left(), right(), zoneId());
     }
 

diff --git a/.../java/org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/NotEquals.java b/.../java/org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/NotEquals.java
@@ -12,8 +12,6 @@
 import org.elasticsearch.xpack.esql.type.EsqlDataTypes;
 import org.elasticsearch.xpack.ql.expression.Expression;
 import org.elasticsearch.xpack.ql.expression.predicate.Negatable;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
-import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
 import org.elasticsearch.xpack.ql.tree.NodeInfo;
 import org.elasticsearch.xpack.ql.tree.Source;
 import org.elasticsearch.xpack.ql.type.DataType;
@@ -22,7 +20,7 @@
 import java.time.ZoneId;
 import java.util.Map;
 
-public class NotEquals extends EsqlBinaryComparison implements Negatable<BinaryComparison> {
+public class NotEquals extends EsqlBinaryComparison implements Negatable<EsqlBinaryComparison> {
     private static final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap = Map.ofEntries(
         Map.entry(DataTypes.BOOLEAN, NotEqualsBoolsEvaluator.Factory::new),
         Map.entry(DataTypes.INTEGER, NotEqualsIntsEvaluator.Factory::new),
@@ -41,11 +39,11 @@ public class NotEquals extends EsqlBinaryComparison implements Negatable<BinaryC
     );
 
     public NotEquals(Source source, Expression left, Expression right) {
-        super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.NEQ, evaluatorMap);
+        super(source, left, right, BinaryComparisonOperation.NEQ, evaluatorMap);
     }
 
     public NotEquals(Source source, Expression left, Expression right, ZoneId zoneId) {
-        super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.NEQ, zoneId, evaluatorMap);
+        super(source, left, right, BinaryComparisonOperation.NEQ, zoneId, evaluatorMap);
     }
 
     @Evaluator(extraName = "Ints")
@@ -79,7 +77,7 @@ static boolean processGeometries(BytesRef lhs, BytesRef rhs) {
     }
 
     @Override
-    public BinaryComparison reverse() {
+    public EsqlBinaryComparison reverse() {
         return this;
     }
 
@@ -99,7 +97,7 @@ public NotEquals swapLeftAndRight() {
     }
 
     @Override
-    public BinaryComparison negate() {
+    public EsqlBinaryComparison negate() {
         return new Equals(source(), left(), right(), zoneId());
     }
 }