SQL: Handle null literal for AND and OR in `WHERE` #35236

matriv · 2018-11-04T15:11:17Z

Eliminate null literal in the Optimizer where we know that we
are in the WHERE clause so we can evaluate NULL to false. Now the
null literal is not reaching the QueryTranslator.

Fixes: #35088

elasticmachine · 2018-11-04T15:11:19Z

Pinging @elastic/es-search-aggs

Translate `null` to a null query and since we know that we are in the WHERE clause so we can evaluate those nulls to false. Fixes: elastic#35088

costin

This needs fixing in a different place.

costin · 2018-11-05T09:52:16Z

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

 }

+ public void testFoldingToLocalExecBooleanAndNull_WhereClause() {
+ PhysicalPlan p = plan("SELECT keyword FROM test WHERE int > 10 AND null");


Looks to me like this should be handled by the optimizer - namely it should evaluate the AND to false.

Darn it, lost my comment - the issue is with And nullable which should return true if either one of its params is nullable not if both are. Once fixed, the nullable rule should kick in, nullify the end which would become a false query which would then be equivalent to false.

If we do that then the <expr> OR null is not foldable so the null reached the QueryTranslator and cannot be translated. The problem is also that with OR we need to partially fold only the null side and eliminate the OR, by returning the non nullable side. It's related to: #35232

as for OR we might fold one of the sides and remove OR and for AND we might need to eliminate it completly.

matriv · 2018-11-06T15:44:57Z

@costin @astefan Implemented the new solution as @costin suggested. Please check again.

costin

The issue could be addressed directly in Or and And nullable(), causing the existing rules to trigger.

costin · 2018-11-07T07:14:52Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

 }
 }

+ static class FoldBinaryLogical implements java.util.function.Function<Expression, Expression> {


No need for a class or instance, simply move the apply(Expression) to a method inside the main class and simply call it or use a method reference.

costin · 2018-11-07T07:16:03Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+ static FoldBinaryLogical INSTANCE = new FoldBinaryLogical();
+
+ @Override
+ public Expression apply(Expression expression) {


Since this method is about nulls it should be inside FoldNull rule. Further more, I don't think there's a need for a rule inside the optimizer, just that Or and And should have the proper nullable() implementations.

The issue is that Or and And cannot be folded in the FoldNull (except if both are null) because we need the 3vl in the select clause. So for example: we cannot fold col1 or null there, because this can potentially return true or null, for and: se cannot fold col1 and null because depending on col1 in can result to false or null. But when we are inside the PruneFilter we know that now we can fold.

costin · 2018-11-07T07:19:08Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+ } else if (e instanceof In) {
 In in = (In) e;
- if (Expressions.isNull(in.value())) {
+ if (canFoldToNullLiteral(in.value())) {


Shouldn't be needed since the fallback if should nullify the value in the first run then fold In in the second.

costin · 2018-11-07T07:20:02Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

- }
-
- if (e.nullable() && Expressions.anyMatch(e.children(), Expressions::isNull)) {
+ } else if (e instanceof BinaryLogic) {


This shouldn't be needed if And/Or.nullable return the proper value.

costin · 2018-11-07T07:20:36Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

 * When encountering a different Equals or non-containing {@link Range}, the conjunction becomes false.
 * When encountering a containing {@link Range}, the range gets eliminated by the equality.
- * 
+ *


I have my editor to trim trailing whitespaces from lines.

Can you please change that to apply only to the modified lines and not the whole fail?
Thanks.
Regardless of the rules chosen, we tend to apply formatting once per project/packages otherwise just for the stuff added.
This reduces the noise when multiple folks work on the same file (like this case).

I have already changed it, i did it once by mistake, will revert the changes.

costin · 2018-11-07T07:21:03Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

 }
 Object eqValue = eq.right().fold();
- 
+


Same here - what's with the whitespace changes?

astefan

LGTM

costin

More comments :)

costin · 2018-11-08T14:18:17Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

 };
-}
+}
+


costin · 2018-11-08T14:19:56Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

 }
+
+
+ static Expression foldBinaryLogic(Expression expression) {


I think it would be better to move this over to the PruneFilter since it's not actually used inside FoldNull.

costin · 2018-11-08T14:22:50Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

- }
-
- if (e.nullable() && Expressions.anyMatch(e.children(), Expressions::isNull)) {
+ } else if (e.nullable() && Expressions.anyMatch(e.children(), Expressions::isNull)) {


Please add a comment or empty line to space out the } else if (...)
The ES formatting uses this style which unfortunately makes less readable the if condition
(it's also why in general I tend to not favor else if.

costin · 2018-11-08T14:26:28Z

...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/logical/And.java

+
+ @Override
+ public boolean nullable() {
+ if (left().nullable() && right().foldable() && right().fold() == Boolean.TRUE) {


Maybe add a static package method on BinaryLogic to avoid doing left/right.foldable() && fold = ? and do if (maybeFoldsTo(right(), Boolean.TRUE) etc...

yep, forgot that, extracting method to Foldables

costin · 2018-11-08T14:32:59Z

...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/logical/And.java

+ return true;
+ }
+
+ if (right().nullable() && left().foldable() && left().fold() == Boolean.TRUE) {


While it might be the logical equivalent, I find the current statement harder to understand.
The logic for AND is: if either side is FALSE, I'm not nullable; otherwise I am if either child is.
For OR it would: if either side is TRUE, I'm not nullable, otherwise the same as above.
In both cases, if the constant checks, there's no nullability check on the children.

The method above checks for the nullability first and then for the folding for the opposite value of each side which I find counter intuitive.

Let's take OR and the rule you propose:

if (Foldables.foldsAndMatches(right(), o -> o == Boolean.TRUE) || Foldables.foldsAndMatches(left(), o -> o == Boolean.TRUE)) { return false; } return left().nullable() || right().nullable();

and the following option:
<expr> OR null => we return true as right is nullable.
Then the FoldNull will check that one of the children is nullable (the right one is) and fold the whole expression to null event if the <expr> evaluates to true.

costin

LGTM! 3VL!

matriv · 2018-11-08T18:53:56Z

retest this please

matriv · 2018-11-08T19:11:29Z

retest this please

Change `nullable()` logic of AND and OR to false since in the Optimizer we cannot fold to null as we might be handling and expression in the SELECT clause. Introduce folding of null for AND and OR expressions in PruneFilter() since we now know that we are in HAVING or WHERE clause and we can fold `null` to `false` Fixes: #35088 Co-authored-by: Costin Leau <costin.leau@gmail.com>

matriv · 2018-11-08T22:53:37Z

Backported to 6.x with dec500d

Change `nullable()` logic of AND and OR to false since in the Optimizer we cannot fold to null as we might be handling and expression in the SELECT clause. Introduce folding of null for AND and OR expressions in PruneFilter() since we now know that we are in HAVING or WHERE clause and we can fold `null` to `false` Fixes: #35088 Co-authored-by: Costin Leau <costin.leau@gmail.com>

matriv · 2018-11-08T23:14:48Z

Backported to 6.5 with 4d0aba2

matriv · 2018-11-09T10:15:35Z

@costin Thx a lot for all your help here!

matriv added >bug v7.0.0 :Analytics/SQL SQL querying v6.6.0 v6.5.1 labels Nov 4, 2018

matriv requested review from astefan and costin November 4, 2018 15:11

matriv removed request for astefan and costin November 4, 2018 15:19

matriv added the WIP label Nov 4, 2018

SQL: Handle null literal for AND and OR in WHERE

ab75b25

Translate `null` to a null query and since we know that we are in the WHERE clause so we can evaluate those nulls to false. Fixes: elastic#35088

matriv force-pushed the mt/fix-35088 branch from a794f04 to ab75b25 Compare November 4, 2018 20:18

matriv removed the WIP label Nov 4, 2018

matriv requested review from astefan and costin November 4, 2018 20:18

costin requested changes Nov 5, 2018

View reviewed changes

matriv added 4 commits November 5, 2018 22:45

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

24f57c1

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

7c86bbc

Handle null folding explicitly in the Optimizer's FoldNull

a66a1c9

as for OR we might fold one of the sides and remove OR and for AND we might need to eliminate it completly.

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

64b6db4

matriv added 2 commits November 6, 2018 21:52

Introduce folding of null for BinaryLogic exprs in PruneFilter()

7f8b2ab

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

b220948

costin requested changes Nov 7, 2018

View reviewed changes

matriv added 5 commits November 7, 2018 19:00

Address comments

2bddbe7

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

9169100

revert whitespace formatting

f408f34

revert whitespace trimming

5d7f638

revert formatting

36951bf

astefan approved these changes Nov 8, 2018

View reviewed changes

costin reviewed Nov 8, 2018

View reviewed changes

matriv added 3 commits November 8, 2018 15:48

Address comments

ee0cceb

fixup! Address comments

4ebc5e4

Simplify rule to always->false

8622588

costin approved these changes Nov 8, 2018

View reviewed changes

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

8800600

matriv merged commit c9a84fe into elastic:master Nov 8, 2018

matriv deleted the mt/fix-35088 branch November 8, 2018 22:33

matriv mentioned this pull request Nov 9, 2018

SQL: Introduce IsNull node to simplify expressions #35206

Merged

colings86 added v6.5.0 and removed v6.5.1 labels Nov 9, 2018

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

SQL: Handle null literal for AND and OR in WHERE #35236

SQL: Handle null literal for AND and OR in WHERE #35236

Uh oh!

Conversation

matriv commented Nov 4, 2018

elasticmachine commented Nov 4, 2018

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Nov 6, 2018

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

matriv commented Nov 8, 2018

matriv commented Nov 8, 2018

matriv commented Nov 8, 2018

matriv commented Nov 8, 2018

matriv commented Nov 9, 2018

Labels

6 participants

SQL: Handle null literal for AND and OR in `WHERE` #35236

SQL: Handle null literal for AND and OR in `WHERE` #35236

matriv Nov 8, 2018 •

edited

Loading