SQL: Check null in processor (elastic/x-pack-elasticsearch#3494)

Make Processors resilient to NULL values
Check null only in functions not constants

Original commit: elastic/x-pack-elasticsearch@dd8bd16d49

qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvSpecTestCase.java

@@ -47,7 +47,8 @@ public abstract class CsvSpecTestCase extends SpecBaseIntegrationTestCase {
                 readScriptSpec("/agg.csv-spec", parser),
                 readScriptSpec("/columns.csv-spec", parser),
                 readScriptSpec("/datetime.csv-spec", parser),
-                readScriptSpec("/alias.csv-spec", parser)
+                readScriptSpec("/alias.csv-spec", parser),
+                readScriptSpec("/nulls.csv-spec", parser)
                 );
     }
 

qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcAssert.java

@@ -7,6 +7,7 @@ package org.elasticsearch.xpack.qa.sql.jdbc;
 
 import org.apache.logging.log4j.Logger;
 import org.relique.jdbc.csv.CsvResultSet;
+
 import java.sql.JDBCType;
 import java.sql.ResultSet;
 import java.sql.ResultSetMetaData;
@@ -112,20 +113,28 @@ public class JdbcAssert {
             for (int column = 1; column <= columns; column++) {
                 Object expectedObject = expected.getObject(column);
                 Object actualObject = actual.getObject(column);
+
                 int type = metaData.getColumnType(column);
 
                 String msg = "Different result for column [" + metaData.getColumnName(column)  + "], entry [" + count + "]";
 
-                if (type == Types.TIMESTAMP || type == Types.TIMESTAMP_WITH_TIMEZONE) {
+                // handle nulls first
+                if (expectedObject == null || actualObject == null) {
+                    assertEquals(expectedObject, actualObject);
+                }
+                // then timestamp
+                else if (type == Types.TIMESTAMP || type == Types.TIMESTAMP_WITH_TIMEZONE) {
                     assertEquals(getTime(expected, column), getTime(actual, column));
                 }
-
+                // and floats/doubles
                 else if (type == Types.DOUBLE) {
                     // the 1d/1f difference is used due to rounding/flooring
                     assertEquals(msg, (double) expectedObject, (double) actualObject, 1d);
                 } else if (type == Types.FLOAT) {
                     assertEquals(msg, (float) expectedObject, (float) actualObject, 1f);
-                } else {
+                }
+                // finally the actual comparison
+                else {
                     assertEquals(msg, expectedObject, actualObject);
                 }
             }

qa/sql/src/main/resources/nulls.csv-spec

@@ -0,0 +1,25 @@
+//
+// Null expressions
+//
+
+nullDate
+SELECT YEAR(CAST(NULL AS DATE)) d;
+
+d:i
+null
+;
+
+nullAdd
+SELECT CAST(NULL AS INT) + CAST(NULL AS FLOAT) AS n;
+
+n:d
+null
+;
+
+
+nullDiv
+SELECT 5 / CAST(NULL AS FLOAT) + 10 AS n;
+
+n:d
+null
+;

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/ArithmeticFunction.java

@@ -13,6 +13,7 @@ import org.elasticsearch.xpack.sql.expression.function.scalar.processor.definiti
 import org.elasticsearch.xpack.sql.expression.function.scalar.script.ScriptTemplate;
 import org.elasticsearch.xpack.sql.tree.Location;
 import org.elasticsearch.xpack.sql.type.DataType;
+import org.elasticsearch.xpack.sql.type.DataTypeConversion;
 
 import java.util.Locale;
 
@@ -34,8 +35,7 @@ public abstract class ArithmeticFunction extends BinaryScalarFunction {
 
     @Override
     public DataType dataType() {
-        // left or right have to be compatible so either one works
-        return left().dataType();
+        return DataTypeConversion.commonType(left().dataType(), right().dataType());
     }
 
     @Override
@@ -72,6 +72,7 @@ public abstract class ArithmeticFunction extends BinaryScalarFunction {
                 .build(), dataType());
     }
 
+    @Override
     protected final BinaryArithmeticProcessorDefinition makeProcessorDefinition() {
         return new BinaryArithmeticProcessorDefinition(this,
                 ProcessorDefinitions.toProcessorDefinition(left()),

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/Arithmetics.java

@@ -12,6 +12,10 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic;
 abstract class Arithmetics {
 
     static Number add(Number l, Number r) {
+        if (l == null || r == null) {
+            return null;
+        }
+
         if (l instanceof Double || r instanceof Double) {
             return Double.valueOf(l.doubleValue() + r.doubleValue());
         }
@@ -26,6 +30,10 @@ abstract class Arithmetics {
     }
 
     static Number sub(Number l, Number r) {
+        if (l == null || r == null) {
+            return null;
+        }
+
         if (l instanceof Double || r instanceof Double) {
             return Double.valueOf(l.doubleValue() - r.doubleValue());
         }
@@ -40,6 +48,10 @@ abstract class Arithmetics {
     }
 
     static Number mul(Number l, Number r) {
+        if (l == null || r == null) {
+            return null;
+        }
+
         if (l instanceof Double || r instanceof Double) {
             return Double.valueOf(l.doubleValue() * r.doubleValue());
         }
@@ -54,6 +66,10 @@ abstract class Arithmetics {
     }
 
     static Number div(Number l, Number r) {
+        if (l == null || r == null) {
+            return null;
+        }
+
         if (l instanceof Double || r instanceof Double) {
             return l.doubleValue() / r.doubleValue();
         }
@@ -68,6 +84,10 @@ abstract class Arithmetics {
     }
 
     static Number mod(Number l, Number r) {
+        if (l == null || r == null) {
+            return null;
+        }
+
         if (l instanceof Long || r instanceof Long) {
             return Long.valueOf(Math.floorMod(l.longValue(), r.longValue()));
         }
@@ -82,6 +102,10 @@ abstract class Arithmetics {
     }
 
     static Number negate(Number n) {
+        if (n == null) {
+            return null;
+        }
+
         if (n instanceof Double) {
             double d = n.doubleValue();
             if (d == Double.MIN_VALUE) {

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/BinaryArithmeticProcessor.java

@@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic;
 
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 import org.elasticsearch.xpack.sql.expression.function.scalar.processor.runtime.BinaryProcessor;
 import org.elasticsearch.xpack.sql.expression.function.scalar.processor.runtime.Processor;
 
@@ -75,6 +76,16 @@ public class BinaryArithmeticProcessor extends BinaryProcessor {
 
     @Override
     protected Object doProcess(Object left, Object right) {
+        if (left == null || right == null) {
+            return null;
+        }
+        if (!(left instanceof Number)) {
+            throw new SqlIllegalArgumentException("A number is required; received %s", left);
+        }
+        if (!(right instanceof Number)) {
+            throw new SqlIllegalArgumentException("A number is required; received %s", right);
+        }
+
         return operation.apply((Number) left, (Number) right);
     }
 

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/UnaryArithmeticProcessor.java

@@ -58,6 +58,10 @@ public class UnaryArithmeticProcessor implements Processor {
 
     @Override
     public Object process(Object input) {
+        if (input == null) {
+            return null;
+        }
+
         if (input instanceof Number) {
             return operation.apply((Number) input);
         }

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFunction.java

@@ -51,6 +51,11 @@ public abstract class DateTimeFunction extends UnaryScalarFunction {
         return field().foldable();
     }
 
+    @Override
+    public Object fold() {
+        return field().fold();
+    }
+
     @Override
     protected TypeResolution resolveType() {
         if (field().dataType().same(DataTypes.DATE)) {

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathProcessor.java

@@ -12,6 +12,7 @@ import org.elasticsearch.xpack.sql.expression.function.scalar.processor.runtime.
 import java.io.IOException;
 import java.util.function.DoubleFunction;
 import java.util.function.Function;
+import java.util.function.Supplier;
 
 public class MathProcessor implements Processor {
     
@@ -35,13 +36,13 @@ public class MathProcessor implements Processor {
         COS(Math::cos),
         COSH(Math::cosh),
         DEGREES(Math::toDegrees),
-        E((Object l) -> Math.E),
+        E(() -> Math.E),
         EXP(Math::exp),
         EXPM1(Math::expm1),
         FLOOR(Math::floor),
         LOG(Math::log),
         LOG10(Math::log10),
-        PI((Object l) -> Math.PI),
+        PI(() -> Math.PI),
         RADIANS(Math::toRadians),
         ROUND((DoubleFunction<Object>) Math::round),
         SIN(Math::sin),
@@ -52,11 +53,15 @@ public class MathProcessor implements Processor {
         private final Function<Object, Object> apply;
 
         MathOperation(Function<Object, Object> apply) {
-            this.apply = apply;
+            this.apply = l -> l == null ? null : apply.apply(l);
         }
 
         MathOperation(DoubleFunction<Object> apply) {
-            this.apply = (Object l) -> apply.apply(((Number) l).doubleValue());
+            this.apply = (Object l) -> l == null ? null : apply.apply(((Number) l).doubleValue());
+        }
+
+        MathOperation(Supplier<Double> supplier) {
+            this.apply = l -> supplier.get();
         }
 
         public final Object apply(Object l) {

sql/server/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java

@@ -306,6 +306,9 @@ public abstract class DataTypeConversion {
         if (detectedType.equals(dataType)) {
             return value;
         }
+        if (value == null) {
+            return value;
+        }
         return conversionFor(detectedType, dataType).convert(value);
     }
 

sql/server/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/BinaryArithmeticProcessorTests.java

@@ -20,7 +20,7 @@ public class BinaryArithmeticProcessorTests extends AbstractWireSerializingTestC
     public static BinaryArithmeticProcessor randomProcessor() {
         return new BinaryArithmeticProcessor(
                 new ConstantProcessor(randomLong()),
-                new ConstantProcessor(randomLong()), 
+                new ConstantProcessor(randomLong()),
                 randomFrom(BinaryArithmeticProcessor.BinaryArithmeticOperation.values()));
     }
 
@@ -82,6 +82,15 @@ public class BinaryArithmeticProcessorTests extends AbstractWireSerializingTestC
         Processor proc = mod.makeProcessorDefinition().asProcessor();
         assertEquals(1, proc.process(null));
     }
+
+    public void testHandleNull() {
+        assertNull(new Add(EMPTY, l(null), l(3)).makeProcessorDefinition().asProcessor().process(null));
+        assertNull(new Sub(EMPTY, l(null), l(3)).makeProcessorDefinition().asProcessor().process(null));
+        assertNull(new Mul(EMPTY, l(null), l(3)).makeProcessorDefinition().asProcessor().process(null));
+        assertNull(new Div(EMPTY, l(null), l(3)).makeProcessorDefinition().asProcessor().process(null));
+        assertNull(new Mod(EMPTY, l(null), l(3)).makeProcessorDefinition().asProcessor().process(null));
+        assertNull(new Neg(EMPTY, l(null)).makeProcessorDefinition().asProcessor().process(null));
+    }
     
     private static Literal l(Object value) {
         return Literal.of(EMPTY, value);