Skip to content

Commit

Permalink
QL: Verify filter's condition type (backport of #66268) (#66408)
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 9fa80c1 commit 176587e
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,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 @@ -146,8 +147,10 @@ Collection<Failure> verify(LogicalPlan plan) {

plan.forEachDown(p -> {
Set<Failure> localFailures = new LinkedHashSet<>();
failures.addAll(localFailures);

checkFilterConditionType(p, localFailures);

failures.addAll(localFailures);
// mark the plan as analyzed
// if everything checks out
if (failures.isEmpty()) {
Expand Down Expand Up @@ -236,4 +239,4 @@ Collection<Failure> verify(LogicalPlan plan) {

return failures;
}
}
}
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 [CONSTANT_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

0 comments on commit 176587e

Please sign in to comment.