Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QL: Verify filter's condition type #66268

Merged
merged 6 commits into from
Dec 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for this test case.
What about covering the simplest case (WHERE boolField) specifically too? The reason: the test case above gets translated into a script query, while the WHERE boolField into a terms query, and while there are plenty of test cases that test the script query, I don't think we have any that tests querying a bool field directly. Note: This would require adding a new field to the test dataset with bool type though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, I think it might make sense to add data for a new test index, with more types (like floats like bools) or extend one of the current ones. But not sure if part of this small PR. I'll open an issue to track it.


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 @@ -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;

Expand Down Expand Up @@ -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<String, List<String>> 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: Condition expression needs to be boolean, found [" + typeName + "]",
error("SELECT * FROM test WHERE " + exp));
}
}
}

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 @@ -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);
}
}