Skip to content

Commit

Permalink
SQL: Verify filter's condition type (backport of #66268) (#66405)
Browse files Browse the repository at this point in the history
* 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 3aec1a3)
  • Loading branch information
bpintea authored Dec 15, 2020
1 parent 8284b93 commit c0c1088
Show file tree
Hide file tree
Showing 7 changed files with 74 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -153,6 +154,7 @@ Collection<Failure> verify(LogicalPlan plan) {
return;
}

checkFilterConditionType(p, localFailures);
checkJoinKeyTypes(p, localFailures);
// mark the plan as analyzed
// if everything checks out
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Failure> 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()));
}
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -208,6 +209,7 @@ Collection<Failure> verify(LogicalPlan plan) {
return;
}

checkFilterConditionType(p, localFailures);
checkGroupingFunctionInGroupBy(p, localFailures);
checkFilterOnAggs(p, localFailures, attributeRefs);
checkFilterOnGrouping(p, localFailures, attributeRefs);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

0 comments on commit c0c1088

Please sign in to comment.