SQL: Implement null handling for `IN(v1, v2, ...)` by matriv · Pull Request #34750 · elastic/elasticsearch

matriv · 2018-10-23T14:11:09Z

Implemented null handling for both the value tested but also for
values inside the list of values tested against.

The null handling is implemented for local processors, painless scripts
and Lucene Terms queries making it available for IN expressions occuring
in SELECT, WHERE and HAVING clauses.

Closes: #34582

Implemented null handling for both the value tested but also for values inside the list of values tested against. The null handling is implemented for local processors, painless scripts and Lucene Terms queries making it available for `IN` expressions occuring in `SELECT`, `WHERE` and `HAVING` clauses. Closes: elastic#34582

elasticmachine · 2018-10-23T14:11:12Z

Pinging @elastic/es-search-aggs

costin

I think the NULL handling inside the values is incorrect since it should signify missing value.
However this can be a separate PR.

costin · 2018-10-23T17:47:04Z

...n/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java

+            if (compResult == null) {
+                result = null;
+            }
            if (compResult != null && compResult) {


compResult is not null already so no need to check it;
if (compResult == null) {..} else if(compResult) { ..}

costin · 2018-10-23T17:49:00Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Foldables.java

+        return valuesOf(list, to, false);
+    }
+
+    public static <T> List<T> valuesOf(List<Expression> list, DataType to, boolean removeNulls) {


I'm not sure why nulls need to be removed - if the expression in the list is NULL, it should be return accordingly.

Please check my comment here: #34750 (comment)

costin · 2018-10-23T17:49:33Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java

            Boolean compResult = Comparisons.eq(foldedLeftValue, rightValue.fold());
-            if (compResult != null && compResult) {
+            if (compResult == null) {
+//                if (value().dataType() == DataType.NULL || rightValue.dataType() == DataType.NULL) {


This comments are confusing (not sure if this is the final version or not).

Sorry about that, some mixup with git stash.

costin · 2018-10-23T17:51:35Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java

        super(location);
        this.term = term;
-        this.values = new LinkedHashSet<>(Foldables.valuesOf(values, values.get(0).dataType()));
+        this.values = new LinkedHashSet<>(Foldables.valuesOf(values, values.get(0).dataType(), true));


To avoid having the overloaded method, it might be more convenient to use Collections.removeIf(p -> p == null) since this is something local to the TermsQuery. On the other hand, it might also imply that when the value is missing (is null), there should be a match.

I did that only to avoid a second iteration on the list (to remove the nulls), but I can change it. what do you think? I guess it can make a difference only if the list is really long.

Yup. I wouldn't worry about it. I'm with you regarding avoiding the iteration but again, the Foldable method looks obscure with the null exclusion.
In other words, it's not Foldable that should handle null - I'm fain with handling it inside Terms I wonder though if that has any implications for inline IN.

costin · 2018-10-23T17:53:47Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java

    }

+    public void testTranslateInExpression_WhereClauseAndNullHAndling() throws IOException {
+        LogicalPlan p = plan("SELECT * FROM test WHERE keyword IN ('foo', null, 'lala', null, 'foo', concat('la', 'la'))");


I think null means the value is missing. That is the IN should become a bool query between missing and terms.

I tried to copy the behaviour to Postgres:

test=# select * from t1 where a in (null); a --- (0 rows) test=# select * from t1 where a is null; a --- (4 rows)

WHERE col IN (null) is different than WHERE col is NULL, the first one evaluates to NULL which in turn becomes false for WHERE and HAVING clauses.

MySQL behaves the same way too.

Makes sense. IN means = and that fails against null (need to use IS (NOT) NULL).

astefan

Looking good. Left just one comment.

astefan · 2018-10-23T18:07:52Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java

+            return true;
        } else if (isString() && other.isString()) {
            return true;
        } else if (isNumeric() && other.isNumeric()) {


All these conditions that return true wouldn't look better if there is one if () only?

I did it only to be more readable, can combine them in one if.

Maybe use one if with one condition evaluation per line - best of both worlds.

Sounds good to me

costin

LGTM

astefan

LGTM

Implemented null handling for both the value tested but also for values inside the list of values tested against. The null handling is implemented for local processors, painless scripts and Lucene Terms queries making it available for `IN` expressions occuring in `SELECT`, `WHERE` and `HAVING` clauses. Closes: #34582

matriv · 2018-10-24T13:18:18Z

Backported to 6.x with 54f3b69

Handle the case when `null` is the only value in the list so that it's translated to a `MatchNoDocsQuery`. Followup to: elastic#34750

Handle the case when `null` is the only value in the list so that it's translated to a `MatchNoDocsQuery`. Followup to: #34750

Implemented null handling for both the value tested but also for values inside the list of values tested against. The null handling is implemented for local processors, painless scripts and Lucene Terms queries making it available for `IN` expressions occuring in `SELECT`, `WHERE` and `HAVING` clauses. Closes: #34582

Handle the case when `null` is the only value in the list so that it's translated to a `MatchNoDocsQuery`. Followup to: #34750

Replace standard `||` and `==` painless operators with `or` and `eq` null safe alternatives from `InternalSqlScriptUtils`. Follow up to elastic#34750

Replace standard `||` and `==` painless operators with new `in` method introduced in `InternalSqlScriptUtils`. This allows the list of values to become a script variable which is replaced each time with the list of values provided by the user. Move In to the same package as InPipe & InProcessor Follow up to #34750 Co-authored-by: Costin Leau <costin.leau@gmail.com>

matriv added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.5.0 labels Oct 23, 2018

matriv requested review from astefan and costin October 23, 2018 14:11

costin reviewed Oct 23, 2018

View reviewed changes

astefan reviewed Oct 24, 2018

View reviewed changes

costin approved these changes Oct 24, 2018

View reviewed changes

astefan approved these changes Oct 24, 2018

View reviewed changes

matriv added 2 commits October 24, 2018 11:23

Address comments

a0bd76c

Merge remote-tracking branch 'upstream/master' into mt/fix-34582-new

5e44251

matriv merged commit 4c73854 into elastic:master Oct 24, 2018

matriv deleted the mt/fix-34582-new branch October 24, 2018 12:42

matriv mentioned this pull request Oct 24, 2018

SQL: Fix edge case: <field> IN (null) #34802

Merged

matriv added a commit to matriv/elasticsearch that referenced this pull request Oct 24, 2018

SQL: Fix edge case: <field> IN (null)

c5c5cfd

Handle the case when `null` is the only value in the list so that it's translated to a `MatchNoDocsQuery`. Followup to: elastic#34750

matriv pushed a commit that referenced this pull request Oct 25, 2018

SQL: Fix edge case: <field> IN (null) (#34802)

bd14333

Handle the case when `null` is the only value in the list so that it's translated to a `MatchNoDocsQuery`. Followup to: #34750

matriv pushed a commit that referenced this pull request Oct 25, 2018

SQL: Fix edge case: <field> IN (null) (#34802)

281b44b

Handle the case when `null` is the only value in the list so that it's translated to a `MatchNoDocsQuery`. Followup to: #34750

matriv pushed a commit that referenced this pull request Oct 25, 2018

SQL: Fix edge case: <field> IN (null) (#34802)

4290f69

Handle the case when `null` is the only value in the list so that it's translated to a `MatchNoDocsQuery`. Followup to: #34750

matriv mentioned this pull request Oct 29, 2018

SQL: Improve painless script generated from IN #35055

Merged

kcm pushed a commit that referenced this pull request Oct 30, 2018

SQL: Fix edge case: <field> IN (null) (#34802)

c61a542

Handle the case when `null` is the only value in the list so that it's translated to a `MatchNoDocsQuery`. Followup to: #34750

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

matriv commented Oct 23, 2018

Uh oh!

elasticmachine commented Oct 23, 2018

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

matriv commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

matriv Oct 24, 2018 •

edited

Loading

matriv Oct 23, 2018 •

edited

Loading