From 6a243239af64f3e975a661745a2d0a6f2a240d08 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Mon, 14 Dec 2020 17:14:20 +0100 Subject: [PATCH 1/5] Verify filter's condition type This adds a check in the verifier to check if filter's condition is of a boolean type and fail the request otherwise. --- .../src/main/resources/conditionals.csv-spec | 17 +++++++++++++++++ .../xpack/sql/analysis/analyzer/Verifier.java | 11 +++++++++++ .../analyzer/VerifierErrorMessagesTests.java | 15 +++++++++++++++ 3 files changed, 43 insertions(+) diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/conditionals.csv-spec b/x-pack/plugin/sql/qa/server/src/main/resources/conditionals.csv-spec index bf72f9958cb97..d0b764e7a4fcb 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/conditionals.csv-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/conditionals.csv-spec @@ -326,6 +326,23 @@ Pettey Heyers ; +iifConditionWhere +SELECT last_name FROM test_emp WHERE IIF(LENGTH(last_name) < 7, true, false) LIMIT 10; + + last_name +----------- +Simmel +Peac +Sluis +Terkki +Genin +Peha +Erde +Famili +Pettey +Heyers +; + iifOrderBy SELECT last_name FROM test_emp ORDER BY IIF(languages >= 3, 'first', 'second'), emp_no LIMIT 10; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index 4d58f0db1f4b3..7c5f3580ed287 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -70,6 +70,7 @@ import static java.util.stream.Collectors.toMap; import static org.elasticsearch.xpack.ql.common.Failure.fail; +import static org.elasticsearch.xpack.ql.type.DataTypes.BOOLEAN; import static org.elasticsearch.xpack.ql.util.CollectionUtils.combine; import static org.elasticsearch.xpack.sql.stats.FeatureMetric.COMMAND; import static org.elasticsearch.xpack.sql.stats.FeatureMetric.GROUPBY; @@ -208,6 +209,7 @@ Collection verify(LogicalPlan plan) { return; } + checkBooleanFiltering(p, localFailures); checkGroupingFunctionInGroupBy(p, localFailures); checkFilterOnAggs(p, localFailures, attributeRefs); checkFilterOnGrouping(p, localFailures, attributeRefs); @@ -626,6 +628,15 @@ private static boolean checkGroupMatch(Expression e, Node source, List localFailures) { + if (p instanceof Filter) { + Expression condition = ((Filter) p).condition(); + if (condition.dataType() != BOOLEAN) { + localFailures.add(fail(condition, "Cannot filter by non-boolean expression of type [{}]", condition.dataType())); + } + } + } + private static void checkGroupingFunctionInGroupBy(LogicalPlan p, Set localFailures) { // check if the query has a grouping function (Histogram) but no GROUP BY if (p instanceof Project) { 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 83e8d52d15f87..23de6c94a5c1c 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 @@ -26,7 +26,9 @@ import org.elasticsearch.xpack.sql.stats.Metrics; import java.util.Arrays; +import java.util.HashMap; import java.util.LinkedHashMap; +import java.util.List; import java.util.Locale; import java.util.Map; @@ -95,6 +97,19 @@ public void testMissingIndex() { assertEquals("1:17: Unknown index [missing]", error(IndexResolution.notFound("missing"), "SELECT foo FROM missing")); } + public void testNonBooleanFilter() { + Map> testData = new HashMap<>(); + testData.put("INTEGER", List.of("int", "int + 1", "ABS(int)", "ASCII(keyword)")); + testData.put("KEYWORD", List.of("keyword", "RTRIM(keyword)", "IIF(true, 'true', 'false')")); + testData.put("DATETIME", List.of("date", "date + INTERVAL 1 DAY", "NOW()")); + for (String typeName : testData.keySet()) { + for (String exp : testData.get(typeName)) { + assertEquals("1:26: Cannot filter by non-boolean expression of type [" + typeName + "]", + error("SELECT * FROM test WHERE " + exp)); + } + } + } + public void testMissingColumn() { assertEquals("1:8: Unknown column [xxx]", error("SELECT xxx FROM test")); } From e91fd4b80d5be5c2ce665836b985ae1b35846c4d Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Mon, 14 Dec 2020 17:52:47 +0100 Subject: [PATCH 2/5] Fix: check type resolution before type's val Make sure a fields's type is available before checking it's value. --- .../org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index 7c5f3580ed287..ef2233c1a9d1e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -631,7 +631,7 @@ private static boolean checkGroupMatch(Expression e, Node source, List localFailures) { if (p instanceof Filter) { Expression condition = ((Filter) p).condition(); - if (condition.dataType() != BOOLEAN) { + if (condition.resolved() && condition.dataType() != BOOLEAN) { localFailures.add(fail(condition, "Cannot filter by non-boolean expression of type [{}]", condition.dataType())); } } From 807b1f047324de09adc27c383085f011e4f1bab7 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Mon, 14 Dec 2020 20:25:20 +0100 Subject: [PATCH 3/5] Adress review comments - remove superfluous resolution check. - rename a method. --- .../xpack/sql/analysis/analyzer/Verifier.java | 8 ++++---- .../sql/analysis/analyzer/VerifierErrorMessagesTests.java | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index ef2233c1a9d1e..9d247509f49d4 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -209,7 +209,7 @@ Collection verify(LogicalPlan plan) { return; } - checkBooleanFiltering(p, localFailures); + checkFilterConditionType(p, localFailures); checkGroupingFunctionInGroupBy(p, localFailures); checkFilterOnAggs(p, localFailures, attributeRefs); checkFilterOnGrouping(p, localFailures, attributeRefs); @@ -628,11 +628,11 @@ private static boolean checkGroupMatch(Expression e, Node source, List localFailures) { + private static void checkFilterConditionType(LogicalPlan p, Set localFailures) { if (p instanceof Filter) { Expression condition = ((Filter) p).condition(); - if (condition.resolved() && condition.dataType() != BOOLEAN) { - localFailures.add(fail(condition, "Cannot filter by non-boolean expression of type [{}]", condition.dataType())); + if (condition.dataType() != BOOLEAN) { + localFailures.add(fail(condition, "Condition expression needs to be boolean, found [{}]", condition.dataType())); } } } 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 23de6c94a5c1c..2aeb316da01b8 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 @@ -104,7 +104,7 @@ public void testNonBooleanFilter() { testData.put("DATETIME", List.of("date", "date + INTERVAL 1 DAY", "NOW()")); for (String typeName : testData.keySet()) { for (String exp : testData.get(typeName)) { - assertEquals("1:26: Cannot filter by non-boolean expression of type [" + typeName + "]", + assertEquals("1:26: Condition expression needs to be boolean, found [" + typeName + "]", error("SELECT * FROM test WHERE " + exp)); } } From 96db9546ee7fa927ad843a5b5e55e159204c630c Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 15 Dec 2020 10:30:48 +0100 Subject: [PATCH 4/5] Extract common SQL-EQL verifier code into class Move checkFilterConditionType() into it's own QL class, VerifierChecks. --- .../xpack/eql/analysis/Verifier.java | 2 ++ .../xpack/eql/analysis/VerifierTests.java | 10 +++++++ .../xpack/ql/analyzer/VerifierChecks.java | 30 +++++++++++++++++++ .../xpack/sql/analysis/analyzer/Verifier.java | 11 +------ 4 files changed, 43 insertions(+), 10 deletions(-) create mode 100644 x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/analyzer/VerifierChecks.java diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Verifier.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Verifier.java index 6a361c5f161d0..7112ef3e5a563 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Verifier.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Verifier.java @@ -53,6 +53,7 @@ import static org.elasticsearch.xpack.eql.stats.FeatureMetric.SEQUENCE_QUERIES_THREE; import static org.elasticsearch.xpack.eql.stats.FeatureMetric.SEQUENCE_QUERIES_TWO; import static org.elasticsearch.xpack.eql.stats.FeatureMetric.SEQUENCE_UNTIL; +import static org.elasticsearch.xpack.ql.analyzer.VerifierChecks.checkFilterConditionType; import static org.elasticsearch.xpack.ql.common.Failure.fail; /** @@ -153,6 +154,7 @@ Collection verify(LogicalPlan plan) { return; } + checkFilterConditionType(p, localFailures); checkJoinKeyTypes(p, localFailures); // mark the plan as analyzed // if everything checks out diff --git a/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/analysis/VerifierTests.java b/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/analysis/VerifierTests.java index e2722f1f1e403..ac9e72a1f997f 100644 --- a/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/analysis/VerifierTests.java +++ b/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/analysis/VerifierTests.java @@ -71,6 +71,16 @@ public void testBasicQuery() { accept("foo where true"); } + public void testQueryCondition() { + accept("any where bool"); + assertEquals("1:11: Condition expression needs to be boolean, found [LONG]", error("any where pid")); + assertEquals("1:11: Condition expression needs to be boolean, found [DATETIME]", error("any where @timestamp")); + assertEquals("1:11: Condition expression needs to be boolean, found [KEYWORD]", error("any where command_line")); + assertEquals("1:11: Condition expression needs to be boolean, found [TEXT]", error("any where hostname")); + assertEquals("1:11: Condition expression needs to be boolean, found [KEYWORD]", error("any where constant_keyword")); + assertEquals("1:11: Condition expression needs to be boolean, found [IP]", error("any where source_address")); + } + public void testQueryStartsWithNumber() { assertEquals("1:1: no viable alternative at input '42'", errorParsing("42 where true")); } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/analyzer/VerifierChecks.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/analyzer/VerifierChecks.java new file mode 100644 index 0000000000000..ac9cd133f2a46 --- /dev/null +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/analyzer/VerifierChecks.java @@ -0,0 +1,30 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.ql.analyzer; + +import org.elasticsearch.xpack.ql.common.Failure; +import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.plan.logical.Filter; +import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan; + +import java.util.Set; + +import static org.elasticsearch.xpack.ql.common.Failure.fail; +import static org.elasticsearch.xpack.ql.type.DataTypes.BOOLEAN; + +public final class VerifierChecks { + + public static void checkFilterConditionType(LogicalPlan p, Set localFailures) { + if (p instanceof Filter) { + Expression condition = ((Filter) p).condition(); + if (condition.dataType() != BOOLEAN) { + localFailures.add(fail(condition, "Condition expression needs to be boolean, found [{}]", condition.dataType())); + } + } + } + +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index 9d247509f49d4..4623fc4083d9b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -69,8 +69,8 @@ import java.util.function.Consumer; import static java.util.stream.Collectors.toMap; +import static org.elasticsearch.xpack.ql.analyzer.VerifierChecks.checkFilterConditionType; import static org.elasticsearch.xpack.ql.common.Failure.fail; -import static org.elasticsearch.xpack.ql.type.DataTypes.BOOLEAN; import static org.elasticsearch.xpack.ql.util.CollectionUtils.combine; import static org.elasticsearch.xpack.sql.stats.FeatureMetric.COMMAND; import static org.elasticsearch.xpack.sql.stats.FeatureMetric.GROUPBY; @@ -628,15 +628,6 @@ private static boolean checkGroupMatch(Expression e, Node source, List localFailures) { - if (p instanceof Filter) { - Expression condition = ((Filter) p).condition(); - if (condition.dataType() != BOOLEAN) { - localFailures.add(fail(condition, "Condition expression needs to be boolean, found [{}]", condition.dataType())); - } - } - } - private static void checkGroupingFunctionInGroupBy(LogicalPlan p, Set localFailures) { // check if the query has a grouping function (Histogram) but no GROUP BY if (p instanceof Project) { From 640ce29879b59d1fac2306d95723456877276296 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 15 Dec 2020 16:17:08 +0100 Subject: [PATCH 5/5] Remove now obsolete test Remove now obsolete test. --- .../xpack/sql/planner/QueryTranslatorTests.java | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index 454548774940b..ec7e3992647db 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -2508,15 +2508,4 @@ public void testAddMissingEqualsToNestedBoolField() { assertEquals(expectedCondition, condition); } - - public void testNotAddMissingEqualsToNonBoolField() { - LogicalPlan p = plan("SELECT bool FROM test WHERE " + randomFrom("int", "text", "keyword", "date")); - assertTrue(p instanceof Project); - - p = ((Project) p).child(); - assertTrue(p instanceof Filter); - - Expression condition = ((Filter) p).condition(); - assertTrue(condition instanceof FieldAttribute); - } }