QL: Verify filter's condition type #66268

bpintea · 2020-12-14T16:24:46Z

This adds a check in the verifier to check if filter's condition is of a
boolean type and fail the request otherwise.

Closes #66254.

This adds a check in the verifier to check if filter's condition is of a boolean type and fail the request otherwise.

Make sure a fields's type is available before checking it's value.

elasticmachine · 2020-12-14T16:54:06Z

Pinging @elastic/es-ql (Team:QL)

costin

Some minor adjustments required. Looks good otherwise.

costin · 2020-12-14T17:05:45Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

@@ -626,6 +628,15 @@ private static boolean checkGroupMatch(Expression e, Node<?> source, List<Expres
        return false;
    }

+    private static void checkBooleanFiltering(LogicalPlan p, Set<Failure> localFailures) {


Filtering only works on bool hence why I recommend renaming it to checkFilterConditionType

costin · 2020-12-14T17:06:31Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

+    private static void checkBooleanFiltering(LogicalPlan p, Set<Failure> localFailures) {
+        if (p instanceof Filter) {
+            Expression condition = ((Filter) p).condition();
+            if (condition.resolved() && condition.dataType() != BOOLEAN) {


No need to check if the condition is resolved, this has already been checked before - these checks get applied only if the plan is fully resolved.

Right, this follows after checking against unresolved nodes (and even a comment about it!). Thanks, fixed.

costin · 2020-12-14T17:09:08Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

+        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()));


Maybe Condition expression needs to be boolean, found [{}]. Remove the notion of filter which is not something the user declares.

palesz

LGTM, one comment regarding testing.

palesz · 2020-12-14T17:29:05Z

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;


👍 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.

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.

palesz

Green checkmark, since my previous comment should not be a blocker.

- remove superfluous resolution check. - rename a method.

Move checkFilterConditionType() into it's own QL class, VerifierChecks.

matriv

LGTM

bpintea · 2020-12-15T10:30:09Z

As @costin pointed out, this check should be extended to EQL as well, so the last commit does that.

The CI should fail until #66252 is merged in though, since with #65325 in place, an = TRUE is added irrespective of the FieldAttribute type (which is corrected by #66252) so any WHERE foo turns into WHERE foo = TRUE which is evaluated as a boolean, so this newly added check incorrectly passes -> turning it to a draft.

bpintea · 2020-12-15T14:29:44Z

@elasticmachine update branch

Remove now obsolete test.

costin

LGTM 2.

* 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)

* 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)

bpintea added 2 commits December 14, 2020 17:14

Verify filter's condition type

6a24323

This adds a check in the verifier to check if filter's condition is of a boolean type and fail the request otherwise.

Fix: check type resolution before type's val

e91fd4b

Make sure a fields's type is available before checking it's value.

bpintea marked this pull request as ready for review December 14, 2020 16:53

bpintea added :Analytics/SQL SQL querying v7.11.0 v8.0.0 labels Dec 14, 2020

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Dec 14, 2020

bpintea requested review from costin, matriv, astefan and palesz and removed request for costin December 14, 2020 16:54

costin added v7.10.2 >bug labels Dec 14, 2020

costin approved these changes Dec 14, 2020

View reviewed changes

palesz reviewed Dec 14, 2020

View reviewed changes

palesz approved these changes Dec 14, 2020

View reviewed changes

bpintea added 2 commits December 14, 2020 20:25

Adress review comments

807b1f0

- remove superfluous resolution check. - rename a method.

Extract common SQL-EQL verifier code into class

96db954

Move checkFilterConditionType() into it's own QL class, VerifierChecks.

matriv approved these changes Dec 15, 2020

View reviewed changes

bpintea marked this pull request as draft December 15, 2020 10:30

bpintea mentioned this pull request Dec 15, 2020

SQL: enhance the test data set with more data types #66325

Closed

elasticmachine and others added 2 commits December 15, 2020 06:29

Merge branch 'master' into feat/verify_non_boolean_cond

b4b8c17

Remove now obsolete test

640ce29

Remove now obsolete test.

bpintea marked this pull request as ready for review December 15, 2020 15:37

bpintea requested a review from costin December 15, 2020 15:37

costin approved these changes Dec 15, 2020

View reviewed changes

bpintea merged commit 3aec1a3 into elastic:master Dec 15, 2020

bpintea deleted the feat/verify_non_boolean_cond branch December 15, 2020 18:47

bpintea added the backport pending label Dec 15, 2020

bpintea changed the title ~~SQL: Verify filter's condition type~~ QL: Verify filter's condition type Dec 15, 2020

bpintea removed the backport pending label Dec 15, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QL: Verify filter's condition type #66268

QL: Verify filter's condition type #66268

bpintea commented Dec 14, 2020

elasticmachine commented Dec 14, 2020

costin left a comment

costin Dec 14, 2020

costin Dec 14, 2020

bpintea Dec 14, 2020

costin Dec 14, 2020

bpintea Dec 14, 2020

palesz left a comment

palesz Dec 14, 2020

bpintea Dec 14, 2020

palesz left a comment

matriv left a comment

bpintea commented Dec 15, 2020

bpintea commented Dec 15, 2020

costin left a comment

QL: Verify filter's condition type #66268

QL: Verify filter's condition type #66268

Conversation

bpintea commented Dec 14, 2020

elasticmachine commented Dec 14, 2020

costin left a comment

Choose a reason for hiding this comment

costin Dec 14, 2020

Choose a reason for hiding this comment

costin Dec 14, 2020

Choose a reason for hiding this comment

bpintea Dec 14, 2020

Choose a reason for hiding this comment

costin Dec 14, 2020

Choose a reason for hiding this comment

bpintea Dec 14, 2020

Choose a reason for hiding this comment

palesz left a comment

Choose a reason for hiding this comment

palesz Dec 14, 2020

Choose a reason for hiding this comment

bpintea Dec 14, 2020

Choose a reason for hiding this comment

palesz left a comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

bpintea commented Dec 15, 2020

bpintea commented Dec 15, 2020

costin left a comment

Choose a reason for hiding this comment