From c0c1088d83ce916ce142beff91cf39a49c255a22 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 15 Dec 2020 22:40:27 +0100 Subject: [PATCH] SQL: Verify filter's condition type (backport of #66268) (#66405) * SQL: Verify filter's condition type (#66268) * 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. (cherry picked from commit 3aec1a3d99a3f4650ec8be014a97106320f0874a) --- .../xpack/eql/analysis/Verifier.java | 2 ++ .../xpack/eql/analysis/VerifierTests.java | 10 +++++++ .../xpack/ql/analyzer/VerifierChecks.java | 30 +++++++++++++++++++ .../src/main/resources/conditionals.csv-spec | 17 +++++++++++ .../xpack/sql/analysis/analyzer/Verifier.java | 2 ++ .../analyzer/VerifierErrorMessagesTests.java | 13 ++++++++ .../sql/planner/QueryTranslatorTests.java | 11 ------- 7 files changed, 74 insertions(+), 11 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/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..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,6 +69,7 @@ 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.util.CollectionUtils.combine; import static org.elasticsearch.xpack.sql.stats.FeatureMetric.COMMAND; @@ -208,6 +209,7 @@ Collection verify(LogicalPlan plan) { return; } + checkFilterConditionType(p, localFailures); checkGroupingFunctionInGroupBy(p, localFailures); checkFilterOnAggs(p, localFailures, attributeRefs); checkFilterOnGrouping(p, localFailures, attributeRefs); 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..a1225928ab180 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 @@ -95,6 +95,19 @@ public void testMissingIndex() { assertEquals("1:17: Unknown index [missing]", error(IndexResolution.notFound("missing"), "SELECT foo FROM missing")); } + public void testNonBooleanFilter() { + String[][] testData = new String[][]{ + {"INTEGER", "int", "int + 1", "ABS(int)", "ASCII(keyword)"}, + {"KEYWORD", "keyword", "RTRIM(keyword)", "IIF(true, 'true', 'false')"}, + {"DATETIME", "date", "date + INTERVAL 1 DAY", "NOW()"}}; + for (String[] testDatum : testData) { + for (int j = 1; j < testDatum.length; j++) { + assertEquals("1:26: Condition expression needs to be boolean, found [" + testDatum[0] + "]", + error("SELECT * FROM test WHERE " + testDatum[j])); + } + } + } + public void testMissingColumn() { assertEquals("1:8: Unknown column [xxx]", error("SELECT xxx FROM test")); } 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 3fee7ff9b2d6c..898c47f655738 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 @@ -2470,15 +2470,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); - } }