From 06e453c9d1eae1d0df6c8f1fc2210e388ee47f6c Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Wed, 27 Nov 2019 10:20:17 +0200 Subject: [PATCH 1/3] Properly handle NULL arithmetic operations with Intervals --- .../main/resources/datetime-interval.csv-spec | 36 +++++++++++++++++ .../xpack/sql/expression/TypeResolutions.java | 7 ++-- .../sql/expression/function/scalar/Cast.java | 3 +- .../predicate/conditional/Case.java | 2 +- .../expression/predicate/nulls/IsNotNull.java | 5 +-- .../expression/predicate/nulls/IsNull.java | 5 +-- .../DateTimeArithmeticOperation.java | 2 +- .../predicate/operator/arithmetic/Mul.java | 6 +-- .../xpack/sql/querydsl/query/TermsQuery.java | 3 +- .../xpack/sql/type/DataType.java | 12 ++++++ .../xpack/sql/type/DataTypeConversion.java | 4 +- .../xpack/sql/type/DataTypes.java | 4 -- .../arithmetic/BinaryArithmeticTests.java | 39 +++++++++++++++++++ 13 files changed, 103 insertions(+), 25 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec index 3a01c7e656563..6c14402189413 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec @@ -142,6 +142,15 @@ INTERVAL 1 DAY + INTERVAL 53 MINUTES +1 00:53:00.0 ; +intervalPlusNull +schema::result:string +SELECT INTERVAL 1 DAY + null AS result; + +result +------------------------------------ +null +; + datePlusIntervalInline SELECT CAST('1969-05-13T12:34:56' AS DATETIME) + INTERVAL 49 YEARS AS result; @@ -167,6 +176,24 @@ SELECT INTERVAL '1' DAY - INTERVAL '2' HOURS AS result; +0 22:00:00.0 ; +intervalMinusNull +schema::result:string +SELECT INTERVAL '1' DAY - null AS result; + + result +--------------- +null +; + +nullMinusInterval +schema::result:string +SELECT null - INTERVAL '1' DAY AS result; + + result +--------------- +null +; + intervalYearMultiply SELECT -2 * INTERVAL '3' YEARS AS result; @@ -184,6 +211,15 @@ SELECT -2 * INTERVAL '1 23:45' DAY TO MINUTES AS result; -3 23:30:00.0 ; +intervalNullMultiply +schema::result:string +SELECT null * INTERVAL '1 23:45' DAY TO MINUTES AS result; + + result +--------------- +null +; + intervalHoursMultiply SELECT 4 * -INTERVAL '2' HOURS AS result1, -5 * -INTERVAL '3' HOURS AS result2; result1 | result2 diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/TypeResolutions.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/TypeResolutions.java index c465ab1b2deb8..30041ea12224b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/TypeResolutions.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/TypeResolutions.java @@ -5,8 +5,9 @@ */ package org.elasticsearch.xpack.sql.expression; +import org.elasticsearch.xpack.sql.expression.Expression.TypeResolution; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.type.DataType; -import org.elasticsearch.xpack.sql.type.DataTypes; import org.elasticsearch.xpack.sql.type.EsField; import java.util.Locale; @@ -14,8 +15,6 @@ import java.util.function.Predicate; import static org.elasticsearch.common.logging.LoggerMessageFormat.format; -import static org.elasticsearch.xpack.sql.expression.Expression.TypeResolution; -import static org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import static org.elasticsearch.xpack.sql.expression.Expressions.name; import static org.elasticsearch.xpack.sql.type.DataType.BOOLEAN; @@ -119,7 +118,7 @@ public static TypeResolution isType(Expression e, String operationName, ParamOrdinal paramOrd, String... acceptedTypes) { - return predicate.test(e.dataType()) || DataTypes.isNull(e.dataType())? + return predicate.test(e.dataType()) || e.dataType().isNull() ? TypeResolution.TYPE_RESOLVED : new TypeResolution(format(null, "{}argument of [{}] must be [{}], found value [{}] type [{}]", paramOrd == null || paramOrd == ParamOrdinal.DEFAULT ? "" : paramOrd.name().toLowerCase(Locale.ROOT) + " ", diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java index fd82d2bb4db23..c3c0da0570903 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java @@ -13,7 +13,6 @@ import org.elasticsearch.xpack.sql.tree.Source; import org.elasticsearch.xpack.sql.type.DataType; import org.elasticsearch.xpack.sql.type.DataTypeConversion; -import org.elasticsearch.xpack.sql.type.DataTypes; import java.util.Objects; @@ -64,7 +63,7 @@ public Object fold() { @Override public Nullability nullable() { - if (DataTypes.isNull(from())) { + if (from().isNull()) { return Nullability.TRUE; } return field().nullable(); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/Case.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/Case.java index 1354bb27034ac..84f17283e061e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/Case.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/Case.java @@ -78,7 +78,7 @@ protected NodeInfo info() { protected TypeResolution resolveType() { DataType expectedResultDataType = null; for (IfConditional ifConditional : conditions) { - if (DataTypes.isNull(ifConditional.result().dataType()) == false) { + if (ifConditional.result().dataType().isNull() == false) { expectedResultDataType = ifConditional.result().dataType(); break; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/IsNotNull.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/IsNotNull.java index f43e12e0b405c..53bf9bcc80504 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/IsNotNull.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/IsNotNull.java @@ -12,10 +12,9 @@ import org.elasticsearch.xpack.sql.expression.gen.script.Scripts; import org.elasticsearch.xpack.sql.expression.predicate.Negatable; import org.elasticsearch.xpack.sql.expression.predicate.nulls.CheckNullProcessor.CheckNullOperation; -import org.elasticsearch.xpack.sql.tree.Source; import org.elasticsearch.xpack.sql.tree.NodeInfo; +import org.elasticsearch.xpack.sql.tree.Source; import org.elasticsearch.xpack.sql.type.DataType; -import org.elasticsearch.xpack.sql.type.DataTypes; public class IsNotNull extends UnaryScalarFunction implements Negatable { @@ -35,7 +34,7 @@ protected IsNotNull replaceChild(Expression newChild) { @Override public Object fold() { - return field().fold() != null && !DataTypes.isNull(field().dataType()); + return field().fold() != null && !field().dataType().isNull(); } @Override diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/IsNull.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/IsNull.java index b873f2770c724..c1d98dbe1b5c5 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/IsNull.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/IsNull.java @@ -12,10 +12,9 @@ import org.elasticsearch.xpack.sql.expression.gen.script.Scripts; import org.elasticsearch.xpack.sql.expression.predicate.Negatable; import org.elasticsearch.xpack.sql.expression.predicate.nulls.CheckNullProcessor.CheckNullOperation; -import org.elasticsearch.xpack.sql.tree.Source; import org.elasticsearch.xpack.sql.tree.NodeInfo; +import org.elasticsearch.xpack.sql.tree.Source; import org.elasticsearch.xpack.sql.type.DataType; -import org.elasticsearch.xpack.sql.type.DataTypes; public class IsNull extends UnaryScalarFunction implements Negatable { @@ -35,7 +34,7 @@ protected IsNull replaceChild(Expression newChild) { @Override public Object fold() { - return field().fold() == null || DataTypes.isNull(field().dataType()); + return field().fold() == null || field().dataType().isNull(); } @Override diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/DateTimeArithmeticOperation.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/DateTimeArithmeticOperation.java index b7b559f1b8617..39797a7351627 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/DateTimeArithmeticOperation.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/DateTimeArithmeticOperation.java @@ -56,7 +56,7 @@ protected TypeResolution resolveWithIntervals() { DataType l = left().dataType(); DataType r = right().dataType(); - if (!(r.isDateOrTimeBased() || r.isInterval())|| !(l.isDateOrTimeBased() || l.isInterval())) { + if (!(r.isDateOrTimeBased() || r.isInterval() || r.isNull())|| !(l.isDateOrTimeBased() || l.isInterval() || l.isNull())) { return new TypeResolution(format(null, "[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r)); } return TypeResolution.TYPE_RESOLVED; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mul.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mul.java index 9c12a24687612..f1e90c2dbd668 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mul.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mul.java @@ -34,14 +34,14 @@ protected TypeResolution resolveType() { DataType r = right().dataType(); // 1. both are numbers - if (l.isNumeric() && r.isNumeric()) { + if (l.isNullOrNumeric() && r.isNullOrNumeric()) { return TypeResolution.TYPE_RESOLVED; } - if (l.isInterval() && r.isInteger()) { + if (l.isNullOrInterval() && (r.isInteger() || r.isNull())) { dataType = l; return TypeResolution.TYPE_RESOLVED; - } else if (r.isInterval() && l.isInteger()) { + } else if (r.isNullOrInterval() && (l.isInteger() || l.isNull())) { dataType = r; return TypeResolution.TYPE_RESOLVED; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java index 966130cb239ec..9b7f59c011ed5 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java @@ -9,7 +9,6 @@ import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Foldables; import org.elasticsearch.xpack.sql.tree.Source; -import org.elasticsearch.xpack.sql.type.DataTypes; import java.util.Collections; import java.util.LinkedHashSet; @@ -27,7 +26,7 @@ public class TermsQuery extends LeafQuery { public TermsQuery(Source source, String term, List values) { super(source); this.term = term; - values.removeIf(e -> DataTypes.isNull(e.dataType())); + values.removeIf(e -> e.dataType().isNull()); if (values.isEmpty()) { this.values = Collections.emptySet(); } else { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java index 22f2a596e3515..19fda28f76ffd 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java @@ -247,6 +247,18 @@ public boolean isSigned() { return isNumeric(); } + public boolean isNull() { + return this == NULL; + } + + public boolean isNullOrNumeric() { + return isNull() || isNumeric(); + } + + public boolean isNullOrInterval() { + return isNull() || isInterval(); + } + public boolean isString() { return this == KEYWORD || this == TEXT; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java index a9b836da1354d..8e3158c3e6a8a 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java @@ -45,10 +45,10 @@ public static DataType commonType(DataType left, DataType right) { if (left == right) { return left; } - if (DataTypes.isNull(left)) { + if (left.isNull()) { return right; } - if (DataTypes.isNull(right)) { + if (right.isNull()) { return left; } if (left.isString() && right.isString()) { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypes.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypes.java index 5d4691e70d531..1faf19e1d0996 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypes.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypes.java @@ -31,10 +31,6 @@ public final class DataTypes { private DataTypes() {} - public static boolean isNull(DataType from) { - return from == NULL; - } - public static boolean isUnsupported(DataType from) { return from == UNSUPPORTED; } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticTests.java index 1c4b0697f959e..1b877cb75f377 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticTests.java @@ -227,6 +227,45 @@ public void testMulNumberInterval() { Period p = interval.interval(); assertEquals(Period.ofYears(2).negated(), p); } + + public void testMulNullInterval() { + Literal literal = interval(Period.ofMonths(1), INTERVAL_MONTH); + Mul result = new Mul(EMPTY, L(null), literal); + assertTrue(result.foldable()); + assertNull(result.fold()); + assertEquals(INTERVAL_MONTH, result.dataType()); + + result = new Mul(EMPTY, literal, L(null)); + assertTrue(result.foldable()); + assertNull(result.fold()); + assertEquals(INTERVAL_MONTH, result.dataType()); + } + + public void testAddNullInterval() { + Literal literal = interval(Period.ofMonths(1), INTERVAL_MONTH); + Add result = new Add(EMPTY, L(null), literal); + assertTrue(result.foldable()); + assertNull(result.fold()); + assertEquals(INTERVAL_MONTH, result.dataType()); + + result = new Add(EMPTY, literal, L(null)); + assertTrue(result.foldable()); + assertNull(result.fold()); + assertEquals(INTERVAL_MONTH, result.dataType()); + } + + public void testSubNullInterval() { + Literal literal = interval(Period.ofMonths(1), INTERVAL_MONTH); + Sub result = new Sub(EMPTY, L(null), literal); + assertTrue(result.foldable()); + assertNull(result.fold()); + assertEquals(INTERVAL_MONTH, result.dataType()); + + result = new Sub(EMPTY, literal, L(null)); + assertTrue(result.foldable()); + assertNull(result.fold()); + assertEquals(INTERVAL_MONTH, result.dataType()); + } @SuppressWarnings("unchecked") private static T add(Object l, Object r) { From fcea46945797c7819379938f7bd10475257f297b Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Wed, 27 Nov 2019 12:15:34 +0200 Subject: [PATCH 2/3] Minor update to Intervals documentation, specifying more clearly which arithmetic operators are supported. --- docs/reference/sql/functions/date-time.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/sql/functions/date-time.asciidoc b/docs/reference/sql/functions/date-time.asciidoc index c3158244ec265..74a58da8fa358 100644 --- a/docs/reference/sql/functions/date-time.asciidoc +++ b/docs/reference/sql/functions/date-time.asciidoc @@ -55,7 +55,7 @@ s|Description ==== Operators -Basic arithmetic operators (`+`, `-`, etc) support date/time parameters as indicated below: +Basic arithmetic operators (`+`, `-`, `*`) support date/time parameters as indicated below: [source, sql] -------------------------------------------------- From d5e97aa6eafa1c2001906f904b5920f71c4aa644 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Wed, 27 Nov 2019 16:22:01 +0200 Subject: [PATCH 3/3] Tests update --- .../qa/src/main/resources/arithmetic.csv-spec | 9 ++++ .../main/resources/datetime-interval.csv-spec | 45 ++++--------------- 2 files changed, 18 insertions(+), 36 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/arithmetic.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/arithmetic.csv-spec index 9cbf544fd8d56..31b2417932aa4 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/arithmetic.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/arithmetic.csv-spec @@ -18,3 +18,12 @@ SELECT 5 - 2 x FROM test_emp LIMIT 5; 3 ; + +nullArithmetics +schema::a:i|b:d|c:s|d:s|e:l|f:i|g:i|h:i|i:i|j:i|k:d +SELECT null + 2 AS a, null * 1.5 AS b, null + null AS c, null - null AS d, null - 1234567890123 AS e, 123 - null AS f, null / 5 AS g, 5 / null AS h, null % 5 AS i, 5 % null AS j, null + 5.5 - (null * (null * 3)) AS k; + + a | b | c | d | e | f | g | h | i | j | k +---------------+---------------+---------------+---------------+---------------+---------------+---------------+---------------+---------------+---------------+--------------- +null |null |null |null |null |null |null |null |null |null |null +; diff --git a/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec index 6c14402189413..8e68410d34786 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec @@ -142,15 +142,6 @@ INTERVAL 1 DAY + INTERVAL 53 MINUTES +1 00:53:00.0 ; -intervalPlusNull -schema::result:string -SELECT INTERVAL 1 DAY + null AS result; - -result ------------------------------------- -null -; - datePlusIntervalInline SELECT CAST('1969-05-13T12:34:56' AS DATETIME) + INTERVAL 49 YEARS AS result; @@ -176,24 +167,6 @@ SELECT INTERVAL '1' DAY - INTERVAL '2' HOURS AS result; +0 22:00:00.0 ; -intervalMinusNull -schema::result:string -SELECT INTERVAL '1' DAY - null AS result; - - result ---------------- -null -; - -nullMinusInterval -schema::result:string -SELECT null - INTERVAL '1' DAY AS result; - - result ---------------- -null -; - intervalYearMultiply SELECT -2 * INTERVAL '3' YEARS AS result; @@ -211,15 +184,6 @@ SELECT -2 * INTERVAL '1 23:45' DAY TO MINUTES AS result; -3 23:30:00.0 ; -intervalNullMultiply -schema::result:string -SELECT null * INTERVAL '1 23:45' DAY TO MINUTES AS result; - - result ---------------- -null -; - intervalHoursMultiply SELECT 4 * -INTERVAL '2' HOURS AS result1, -5 * -INTERVAL '3' HOURS AS result2; result1 | result2 @@ -227,6 +191,15 @@ SELECT 4 * -INTERVAL '2' HOURS AS result1, -5 * -INTERVAL '3' HOURS AS result2; -0 08:00:00.0 | +0 15:00:00.0 ; +intervalNullMath +schema::null_multiply:string|null_sub1:string|null_sub2:string|null_add:string +SELECT null * INTERVAL '1 23:45' DAY TO MINUTES AS null_multiply, INTERVAL '1' DAY - null AS null_sub1, null - INTERVAL '1' DAY AS null_sub2, INTERVAL 1 DAY + null AS null_add; + + null_multiply | null_sub1 | null_sub2 | null_add +-----------------+-------------+-------------+------------- +null |null |null |null +; + intervalAndFieldMultiply schema::languages:byte|result:string SELECT languages, CAST (languages * INTERVAL '1 10:30' DAY TO MINUTES AS string) AS result FROM test_emp ORDER BY emp_no LIMIT 5;