From 542d1ee66ce409c48e6f20af6bdac85745c6fa2f Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 9 May 2019 16:16:03 +0300 Subject: [PATCH 1/3] SQL: Fix issue regarding INTERVAL * number Interval * integer number is a valid operation which previously was only supported for foldables (literals) and not when a field was involved. That was because: 1. There was no common type returned for that combination 2. The `BinaryArithmeticOperation` was permitting the multiplication (called by fold()) but the BinaryArithmeticProcessor didn't allow it Moreover the error message for invalid arithmetic operations was wrong because of the issue with the overloading methods of `LoggerMessageFormat.format`. Fixes: #41239 --- .../src/main/resources/datetime-interval.csv-spec | 13 +++++++++++++ .../arithmetic/BinaryArithmeticProcessor.java | 6 +++--- .../arithmetic/DateTimeArithmeticOperation.java | 11 ++++++++++- .../predicate/operator/arithmetic/Mul.java | 2 +- .../xpack/sql/type/DataTypeConversion.java | 11 +++++++++++ .../analyzer/VerifierErrorMessagesTests.java | 15 +++++++++++++++ .../xpack/sql/type/DataTypeConversionTests.java | 4 ++++ 7 files changed, 57 insertions(+), 5 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 8d9a65d1b85b6..0f656bd9cb271 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 @@ -182,6 +182,19 @@ SELECT -2 * INTERVAL '1 23:45' DAY TO MINUTES AS result; -3 23:30:00.0 ; +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; + + languages | result +---------------+--------------------------------------------- +2 | +2 21:00:00.0 +5 | +7 04:30:00.0 +4 | +5 18:00:00.0 +5 | +7 04:30:00.0 +1 | +1 10:30:00.0 +; + dateMinusInterval SELECT CAST('2018-05-13T12:34:56' AS DATETIME) - INTERVAL '2-8' YEAR TO MONTH AS result; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticProcessor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticProcessor.java index b6bfaa4acb63d..5705bb4d85ab4 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticProcessor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticProcessor.java @@ -164,7 +164,7 @@ protected Object doProcess(Object left, Object right) { return null; } - if (f == BinaryArithmeticOperation.MUL || f == BinaryArithmeticOperation.DIV || f == BinaryArithmeticOperation.MOD) { + if (f == BinaryArithmeticOperation.DIV || f == BinaryArithmeticOperation.MOD) { if (!(left instanceof Number)) { throw new SqlIllegalArgumentException("A number is required; received {}", left); } @@ -176,8 +176,8 @@ protected Object doProcess(Object left, Object right) { return f.apply(left, right); } - if (f == BinaryArithmeticOperation.ADD || f == BinaryArithmeticOperation.SUB) { - return f.apply(left, right); + if (f == BinaryArithmeticOperation.ADD || f == BinaryArithmeticOperation.SUB || f == BinaryArithmeticOperation.MUL) { + return f.apply(left, right); } // this should not occur 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 5be5e28718459..23219f8209ed6 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 @@ -43,7 +43,7 @@ protected TypeResolution resolveType() { // 2. 3. 4. intervals if ((DataTypes.isInterval(l) || DataTypes.isInterval(r))) { if (DataTypeConversion.commonType(l, r) == null) { - return new TypeResolution(format("[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r)); + return new TypeResolution(format(null, "[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r)); } else { return resolveWithIntervals(); } @@ -54,6 +54,15 @@ protected TypeResolution resolveType() { } protected TypeResolution resolveWithIntervals() { + DataType l = left().dataType(); + DataType r = right().dataType(); + + if (!(r.isDateOrTimeBased() || DataTypes.isInterval(r))) { + return new TypeResolution(format(null, "[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r)); + } + if (!(l.isDateOrTimeBased() || DataTypes.isInterval(l))) { + 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 7a09bbedebfa3..e3fa7ac1031f7 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 @@ -47,7 +47,7 @@ protected TypeResolution resolveType() { return TypeResolution.TYPE_RESOLVED; } - return new TypeResolution(format("[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r)); + return new TypeResolution(format(null, "[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r)); } @Override 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 40a03e26eb0ef..5fd1867aeb27a 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 @@ -121,6 +121,17 @@ public static DataType commonType(DataType left, DataType right) { return right; } } + // Interval * integer is a valid operation + if (DataTypes.isInterval(left)) { + if (right.isInteger()) { + return left; + } + } + if (DataTypes.isInterval(right)) { + if (left.isInteger()) { + return right; + } + } if (DataTypes.isInterval(left)) { // intervals widening if (DataTypes.isInterval(right)) { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index dcf8dad5ecb79..17cc8f3a664a3 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -241,6 +241,21 @@ public void testSubtractFromInterval() { error("SELECT INTERVAL 1 MONTH - CAST('12:23:56.789' AS TIME)")); } + public void testAddIntervalAndNumberNotAllowed() { + assertEquals("1:8: [+] has arguments with incompatible types [INTERVAL_DAY] and [INTEGER]", + error("SELECT INTERVAL 1 DAY + 100")); + } + + public void testSubtractIntervalAndNumberNotAllowed() { + assertEquals("1:8: [-] has arguments with incompatible types [INTERVAL_MINUTE] and [DOUBLE]", + error("SELECT INTERVAL 10 MINUTE - 100.0")); + } + + public void testMultiplyIntervalWithDecimalNotAllowed() { + assertEquals("1:8: [*] has arguments with incompatible types [INTERVAL_MONTH] and [DOUBLE]", + error("SELECT INTERVAL 1 MONTH * 1.234")); + } + public void testMultipleColumns() { assertEquals("1:43: Unknown column [xxx]\nline 1:8: Unknown column [xxx]", error("SELECT xxx FROM test GROUP BY DAY_oF_YEAR(xxx)")); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypeConversionTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypeConversionTests.java index 447c820c8e421..7ca4d0058325f 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypeConversionTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypeConversionTests.java @@ -628,6 +628,10 @@ public void testCommonType() { assertEquals(FLOAT, commonType(FLOAT, INTEGER)); assertEquals(DOUBLE, commonType(DOUBLE, FLOAT)); + // numeric and intervals + assertEquals(INTERVAL_YEAR_TO_MONTH, commonType(INTERVAL_YEAR_TO_MONTH, LONG)); + assertEquals(INTERVAL_HOUR_TO_MINUTE, commonType(INTEGER, INTERVAL_HOUR_TO_MINUTE)); + // dates/datetimes and intervals assertEquals(DATETIME, commonType(DATE, DATETIME)); assertEquals(DATETIME, commonType(DATETIME, DATE)); From 3411a84b1173c6fceda75b31b42c2492e4fa07ff Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Fri, 10 May 2019 19:14:36 +0300 Subject: [PATCH 2/3] Address comment --- .../operator/arithmetic/DateTimeArithmeticOperation.java | 5 +---- .../sql/expression/predicate/operator/arithmetic/Sub.java | 4 ++++ .../sql/analysis/analyzer/VerifierErrorMessagesTests.java | 6 ++++++ 3 files changed, 11 insertions(+), 4 deletions(-) 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 23219f8209ed6..5b1076592d859 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 @@ -57,10 +57,7 @@ protected TypeResolution resolveWithIntervals() { DataType l = left().dataType(); DataType r = right().dataType(); - if (!(r.isDateOrTimeBased() || DataTypes.isInterval(r))) { - return new TypeResolution(format(null, "[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r)); - } - if (!(l.isDateOrTimeBased() || DataTypes.isInterval(l))) { + if (!(r.isDateOrTimeBased() || DataTypes.isInterval(r))|| !(l.isDateOrTimeBased() || DataTypes.isInterval(l))) { 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/Sub.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java index ee3ca6aa6773b..a47b9cc973122 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java @@ -34,6 +34,10 @@ protected Sub replaceChildren(Expression newLeft, Expression newRight) { @Override protected TypeResolution resolveWithIntervals() { + TypeResolution resolution = super.resolveWithIntervals(); + if (resolution.unresolved()) { + return resolution; + } if ((right().dataType().isDateOrTimeBased()) && DataTypes.isInterval(left().dataType())) { return new TypeResolution(format(null, "Cannot subtract a {}[{}] from an interval[{}]; do you mean the reverse?", right().dataType().typeName, right().source().text(), left().source().text())); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 17cc8f3a664a3..ddd9371d1a58a 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -244,16 +244,22 @@ public void testSubtractFromInterval() { public void testAddIntervalAndNumberNotAllowed() { assertEquals("1:8: [+] has arguments with incompatible types [INTERVAL_DAY] and [INTEGER]", error("SELECT INTERVAL 1 DAY + 100")); + assertEquals("1:8: [+] has arguments with incompatible types [INTEGER] and [INTERVAL_DAY]", + error("SELECT 100 + INTERVAL 1 DAY")); } public void testSubtractIntervalAndNumberNotAllowed() { assertEquals("1:8: [-] has arguments with incompatible types [INTERVAL_MINUTE] and [DOUBLE]", error("SELECT INTERVAL 10 MINUTE - 100.0")); + assertEquals("1:8: [-] has arguments with incompatible types [DOUBLE] and [INTERVAL_MINUTE]", + error("SELECT 100.0 - INTERVAL 10 MINUTE")); } public void testMultiplyIntervalWithDecimalNotAllowed() { assertEquals("1:8: [*] has arguments with incompatible types [INTERVAL_MONTH] and [DOUBLE]", error("SELECT INTERVAL 1 MONTH * 1.234")); + assertEquals("1:8: [*] has arguments with incompatible types [DOUBLE] and [INTERVAL_MONTH]", + error("SELECT 1.234 * INTERVAL 1 MONTH")); } public void testMultipleColumns() { From 7394e805240aa45b14df96f928fcdeceff98d9e3 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 13 May 2019 00:26:45 +0200 Subject: [PATCH 3/3] add more tests --- .../sql/qa/src/main/resources/datetime-interval.csv-spec | 7 +++++++ 1 file changed, 7 insertions(+) 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 0f656bd9cb271..bfb28775bc3b6 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 @@ -182,6 +182,13 @@ SELECT -2 * INTERVAL '1 23:45' DAY TO MINUTES AS result; -3 23:30:00.0 ; +intervalHoursMultiply +SELECT 4 * -INTERVAL '2' HOURS AS result1, -5 * -INTERVAL '3' HOURS AS result2; + result1 | result2 +---------------+-------------- +-0 08:00:00.0 | +0 15:00:00.0 +; + 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;