From ca8f8b4a13931b64ebc2ff0fd8bb073367568f8f Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 25 Nov 2019 15:36:25 +0100 Subject: [PATCH 1/3] SQL: Fix issue with CASE/IIF pre-calculating results Previously, CaseProcessor was pre-calculating (called process()) on all the building elements of a CASE/IIF expression, not only the conditions involved but also the results (if true/if false), as well as the final else result. In case one of those results had an erroneous calculation (e.g.: division by zero) this was executed and resulted in an Exception to be thrown, even if this result was not used because of the condition guarding it. e.g.: ``` SELECT CASE myField1 = 0 THEN NULL ELSE myField2 / myField1 END FROM test; ``` Fixes: #49388 --- .../src/main/resources/conditionals.csv-spec | 26 +++++++++++++++++++ .../plugin/sql/qa/src/main/resources/logs.csv | 2 +- .../predicate/conditional/CaseProcessor.java | 11 ++++---- 3 files changed, 33 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec index aaa29e814d4b3..6eed7187a8ca7 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec @@ -69,6 +69,19 @@ FROM test_emp ORDER BY 1 LIMIT 5; 10005 | 1 | 10005 ; +caseWithErroneousResultsForFalseConditions +schema::bytes_in:i|bytes_out:i|div:i +SELECT bytes_in, bytes_out, CASE WHEN bytes_in = 0 THEN NULL ELSE bytes_out / bytes_in END div FROM logs ORDER BY bytes_in LIMIT 5; + + bytes_in | bytes_out | div +---------------+---------------+--------------- +0 |128 |null +0 |null |null +8 |null |null +8 |null |null +8 |null |null +; + caseWhere SELECT last_name FROM test_emp WHERE CASE WHEN LENGTH(last_name) < 7 THEN 'ShortName' ELSE 'LongName' END = 'LongName' ORDER BY emp_no LIMIT 10; @@ -265,6 +278,19 @@ SELECT emp_no, IIF(NULL, emp_no) AS IIF_1, IIF(NULL, emp_no, languages) AS IIF_2 10005 | null | 1 | 10005 ; +iifWithErroneousResultsForFalseCondition +schema::bytes_in:i|bytes_out:i|div:i +SELECT bytes_in, bytes_out, IIF(bytes_in = 0, NULL, bytes_out / bytes_in) div FROM logs ORDER BY bytes_in LIMIT 5; + + bytes_in | bytes_out | div +---------------+---------------+--------------- +0 |128 |null +0 |null |null +8 |null |null +8 |null |null +8 |null |null +; + iifWhere SELECT last_name FROM test_emp WHERE IIF(LENGTH(last_name) < 7, 'ShortName') IS NOT NULL ORDER BY emp_no LIMIT 10; diff --git a/x-pack/plugin/sql/qa/src/main/resources/logs.csv b/x-pack/plugin/sql/qa/src/main/resources/logs.csv index 240fb3752ab53..7103f578b80b6 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/logs.csv +++ b/x-pack/plugin/sql/qa/src/main/resources/logs.csv @@ -25,7 +25,7 @@ id,@timestamp,bytes_in,bytes_out,client_ip,client_port,dest_ip,status 24,2017-11-10T20:34:43Z,8,,10.0.1.166,,2001:cafe::13e1:16fc:8726:1bf8,OK 25,2017-11-10T23:30:46Z,40,,10.0.1.199,,2001:cafe::ff07:bdcc:bc59:ff9f,OK 26,2017-11-10T21:13:16Z,20,,,,2001:cafe::ff07:bdcc:bc59:ff9f,OK -27,2017-11-10T23:36:32Z,0,,10.0.1.199,,2001:cafe::13e1:16fc:8726:1bf8,OK +27,2017-11-10T23:36:32Z,0,128,10.0.1.199,,2001:cafe::13e1:16fc:8726:1bf8,OK 28,2017-11-10T23:36:33Z,40,,10.0.1.199,,2001:cafe::ff07:bdcc:bc59:ff9f,OK 29,2017-11-10T20:35:26Z,20,,10.0.1.166,,2001:cafe::ff07:bdcc:bc59:ff9f,OK 30,2017-11-10T23:36:41Z,8,,,,2001:cafe::13e1:16fc:8726:1bf8,OK diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CaseProcessor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CaseProcessor.java index 634e83401fe63..d86f17199cc16 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CaseProcessor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CaseProcessor.java @@ -10,7 +10,6 @@ import org.elasticsearch.xpack.sql.expression.gen.processor.Processor; import java.io.IOException; -import java.util.ArrayList; import java.util.List; import java.util.Objects; @@ -40,11 +39,13 @@ public void writeTo(StreamOutput out) throws IOException { @Override public Object process(Object input) { - List objects = new ArrayList<>(processors.size()); - for (Processor processor : processors) { - objects.add(processor.process(input)); + for (int i = 0; i < processors.size() - 2; i += 2) { + if (processors.get(i).process(input)==Boolean.TRUE) { + return processors.get(i + 1).process(input); + } } - return apply(objects); + // resort to default value + return processors.get(processors.size() - 1).process(input); } public static Object apply(List objects) { From 6aafde9bf02288b6d49bf2cd3406d84c935900ee Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 25 Nov 2019 16:59:12 +0100 Subject: [PATCH 2/3] Address comments --- .../sql/qa/src/main/resources/conditionals.csv-spec | 10 +++++----- .../predicate/conditional/CaseProcessor.java | 6 +++++- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec index 6eed7187a8ca7..c9f8d99e60004 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec @@ -71,15 +71,15 @@ FROM test_emp ORDER BY 1 LIMIT 5; caseWithErroneousResultsForFalseConditions schema::bytes_in:i|bytes_out:i|div:i -SELECT bytes_in, bytes_out, CASE WHEN bytes_in = 0 THEN NULL ELSE bytes_out / bytes_in END div FROM logs ORDER BY bytes_in LIMIT 5; +SELECT bytes_in, bytes_out, CASE WHEN bytes_in = 0 THEN NULL WHEN bytes_in < 10 THEN bytes_in * 20 ELSE bytes_out / bytes_in END div FROM logs ORDER BY bytes_in LIMIT 5; bytes_in | bytes_out | div ---------------+---------------+--------------- 0 |128 |null 0 |null |null -8 |null |null -8 |null |null -8 |null |null +8 |null |160 +8 |null |160 +8 |null |160 ; caseWhere @@ -280,7 +280,7 @@ SELECT emp_no, IIF(NULL, emp_no) AS IIF_1, IIF(NULL, emp_no, languages) AS IIF_2 iifWithErroneousResultsForFalseCondition schema::bytes_in:i|bytes_out:i|div:i -SELECT bytes_in, bytes_out, IIF(bytes_in = 0, NULL, bytes_out / bytes_in) div FROM logs ORDER BY bytes_in LIMIT 5; +SELECT bytes_in, bytes_out, IIF(bytes_in < 10, IIF(bytes_in = 0, NULL, bytes_in * 10), bytes_out / bytes_in) div FROM logs ORDER BY bytes_in LIMIT 5; bytes_in | bytes_out | div ---------------+---------------+--------------- diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CaseProcessor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CaseProcessor.java index d86f17199cc16..269faba3dc9e1 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CaseProcessor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CaseProcessor.java @@ -39,8 +39,10 @@ public void writeTo(StreamOutput out) throws IOException { @Override public Object process(Object input) { + // Check every condition in sequence and if it evaluates to TRUE, + // evaluate and return the result associated with that condition. for (int i = 0; i < processors.size() - 2; i += 2) { - if (processors.get(i).process(input)==Boolean.TRUE) { + if (processors.get(i).process(input) == Boolean.TRUE) { return processors.get(i + 1).process(input); } } @@ -49,6 +51,8 @@ public Object process(Object input) { } public static Object apply(List objects) { + // Check every condition in sequence and if it evaluates to TRUE, + // evaluate and return the result associated with that condition. for (int i = 0; i < objects.size() - 2; i += 2) { if (objects.get(i) == Boolean.TRUE) { return objects.get(i + 1); From ca078ebe7b551482d534d91aff48a30ff0464a54 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 25 Nov 2019 18:11:25 +0100 Subject: [PATCH 3/3] fix integ test --- .../plugin/sql/qa/src/main/resources/conditionals.csv-spec | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec index c9f8d99e60004..9f424da2710e3 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec @@ -286,9 +286,9 @@ SELECT bytes_in, bytes_out, IIF(bytes_in < 10, IIF(bytes_in = 0, NULL, bytes_in ---------------+---------------+--------------- 0 |128 |null 0 |null |null -8 |null |null -8 |null |null -8 |null |null +8 |null |80 +8 |null |80 +8 |null |80 ; iifWhere