From 25a00a3b55010f58da0047f29cf700cec5c091e5 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Tue, 9 Jan 2018 16:18:16 +0200 Subject: [PATCH] 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@dd8bd16d49cc7bd7bf77925bd389f83cd4a15e18 --- .../xpack/qa/sql/jdbc/CsvSpecTestCase.java | 3 ++- .../xpack/qa/sql/jdbc/JdbcAssert.java | 15 ++++++++--- qa/sql/src/main/resources/nulls.csv-spec | 25 +++++++++++++++++++ .../scalar/arithmetic/ArithmeticFunction.java | 5 ++-- .../scalar/arithmetic/Arithmetics.java | 24 ++++++++++++++++++ .../arithmetic/BinaryArithmeticProcessor.java | 11 ++++++++ .../arithmetic/UnaryArithmeticProcessor.java | 4 +++ .../scalar/datetime/DateTimeFunction.java | 5 ++++ .../function/scalar/math/MathProcessor.java | 13 +++++++--- .../xpack/sql/type/DataTypeConversion.java | 3 +++ .../BinaryArithmeticProcessorTests.java | 11 +++++++- 11 files changed, 108 insertions(+), 11 deletions(-) create mode 100644 qa/sql/src/main/resources/nulls.csv-spec diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvSpecTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvSpecTestCase.java index f7b15d8283e..85b972fc84a 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvSpecTestCase.java +++ b/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) ); } diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcAssert.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcAssert.java index 16b783f21fa..e3455c18d1a 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcAssert.java +++ b/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); } } diff --git a/qa/sql/src/main/resources/nulls.csv-spec b/qa/sql/src/main/resources/nulls.csv-spec new file mode 100644 index 00000000000..1cb9a1ed7f3 --- /dev/null +++ b/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 +; diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/ArithmeticFunction.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/ArithmeticFunction.java index 1d6d4b7c245..b7cf36dd756 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/ArithmeticFunction.java +++ b/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()), diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/Arithmetics.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/Arithmetics.java index 2bae1935e4f..51cccb85066 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/Arithmetics.java +++ b/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) { diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/BinaryArithmeticProcessor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/BinaryArithmeticProcessor.java index bfb4cd811cb..be357a3e808 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/BinaryArithmeticProcessor.java +++ b/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); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/UnaryArithmeticProcessor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/UnaryArithmeticProcessor.java index cc60a8c5004..4ed923d2af9 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/UnaryArithmeticProcessor.java +++ b/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); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFunction.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFunction.java index fa39f5ec62d..302defd6017 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFunction.java +++ b/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)) { diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathProcessor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathProcessor.java index 1548de6704a..5336ff66afc 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathProcessor.java +++ b/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) Math::round), SIN(Math::sin), @@ -52,11 +53,15 @@ public class MathProcessor implements Processor { private final Function apply; MathOperation(Function apply) { - this.apply = apply; + this.apply = l -> l == null ? null : apply.apply(l); } MathOperation(DoubleFunction apply) { - this.apply = (Object l) -> apply.apply(((Number) l).doubleValue()); + this.apply = (Object l) -> l == null ? null : apply.apply(((Number) l).doubleValue()); + } + + MathOperation(Supplier supplier) { + this.apply = l -> supplier.get(); } public final Object apply(Object l) { diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java index 0040848e5a9..36f2311a9cf 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java +++ b/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); } diff --git a/sql/server/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/BinaryArithmeticProcessorTests.java b/sql/server/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/BinaryArithmeticProcessorTests.java index 25e46d1774b..08736caef66 100644 --- a/sql/server/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/BinaryArithmeticProcessorTests.java +++ b/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);