SQL: Introduce IsNull node to simplify expressions #35206

matriv · 2018-11-02T13:16:41Z

Add IsNull node in parser to simplify expressions so that <value> IS NULL is
no longer translated internally to NOT(<value> IS NOT NULL)

Replace IsNotNullProcessor with CheckNullProcessor to encapsulate both
isNull and isNotNull functionality.

Closes: #34876
Fixes: #35171

Add `IsNull` node in parser to simplify expressions so that `<value> IS NULL` is no longer translated internally to `NOT(<value> IS NOT NULL)` Closes: elastic#34876

elasticmachine · 2018-11-02T13:16:43Z

Pinging @elastic/es-search-aggs

matriv · 2018-11-02T13:17:24Z

Integration tests for HAVING will be introduced together with the fix here: https://github.com/elastic/elasticsearch/pull/35164/files#diff-a3ba4284670bdc13a2ef764cff91a212

matriv · 2018-11-02T13:32:07Z

retest this please

costin

Looks good however the change is too broad (see my comments on negatable) hence my requests for changes.

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

...ugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/IsNullProcessor.java

+
+import java.io.IOException;
+
+public class IsNullProcessor implements Processor {


As we have two very similar processors, it makes sense to aggregate the isnull/isnotnull processors into a single processor class which uses an enum to differentiate between the two (as we do with the rest of other operators).
This makes things consistent and keeps the serialization name explosion to a minimum.
It's also a good reason to keep this a 6.6 feature and not backport it to 6.5 (since it's an enhancement not really a bug).

I've tested this PR for this bug - #35171 and it's no longer reproduceable... might be a good reason to backport it to 6.5 @costin ?

If that's the case and the change isn't too big, it's worth backporting yes.

costin · 2018-11-05T09:18:24Z

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

                new ConstantFolding(),
                // boolean
                new BooleanSimplification(),
                new BooleanLiteralsOnTheRight(),
                new BinaryComparisonSimplification(),
+                // need to occur after simplification


need to occur after simplification

Why? It should get picked up on the next cycle in the worse case scenario.

costin · 2018-11-05T09:24:54Z

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

@@ -58,8 +57,8 @@ public String processScript(String script) {
    @Override
    protected Expression canonicalize() {
        Expression canonicalChild = field().canonical();
-        if (canonicalChild instanceof Negateable) {
-            return ((Negateable) canonicalChild).negate();
+        if (canonicalChild instanceof UnaryNegateable) {


This doesn't look good as it essentially removes NOT over binary logical negateable NOT (x AND y).

That's a mistake for sure, thx for catching!

Can you add some tests please with NOT in HAVING over AND and IS NULL to check this automatically in the future?

costin · 2018-11-05T09:30:01Z

...rc/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/UnaryScalarFunction.java

@@ -19,6 +19,10 @@

 public abstract class UnaryScalarFunction extends ScalarFunction {

+    public interface UnaryNegateable {


Regardless of the number of children, there should be a negatable (I believe that's the correct syntax) interface common to all. In other words, Negatable should be moved up from BinaryOperator to potentially a standalone interface and likely have its generic signature extended to bound its return type (Negatable<T extends ScalarFunction>).

costin · 2018-11-05T09:34:35Z

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

@@ -1278,8 +1284,11 @@ private Expression simplifyNot(Not n) {
                return TRUE;
            }

-            if (c instanceof Negateable) {
-                return ((Negateable) c).negate();
+            if (c instanceof BinaryNegateable) {


Case in point, it doesn't make any difference internally whether an expression is BinaryNegtable or Unary - the result in both cases is an Expression so why differentiate between the two.

It was just because of the 2 different ifaces, but as you suggested we'd better merge them into one.

costin

Made some comments. Thanks.

costin · 2018-11-07T07:37:39Z

...gin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Negateable.java

+package org.elasticsearch.xpack.sql.expression.function.scalar;
+
+public interface Negateable {
+


No need to extract the interface so high up - just put it inside the predicate package since essentially it applies only to logical predicates. Also the correct name should be Negatable.

Also it's worth looking into adding the generic aspect in place to have a better lock down on the class hierarchy:
Negatable<T extends ScalarFunction> { T negate() } . IsNull/IsNotNull can lock it to UnaryScalarFunction while the BinaryLogic keep the BinaryOperator in place.

But i didn't change its name: Negate + able = Negateable :) but the correct word is negatable, so I'll change.

costin · 2018-11-07T07:39:47Z

...src/main/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/CheckNullProcessor.java

+    public enum CheckNullOperation implements Function<Object, Boolean> {
+
+        IS_NULL(Objects::isNull, "IS NULL"),
+        IS_NOT_NULL(o -> !Objects.isNull(o), "IS NOT NULL");


Use Objects::nonNull (the main reason these methods exist on Objects).

costin · 2018-11-07T07:41:50Z

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

                new ConstantFolding(),
+                new FoldNull(),


Again, why was FoldNull moved?

no reason, missed the initial place, will change.

matriv · 2018-11-09T01:08:05Z

@costin @astefan Addressed comments and fixed things after syncing with #35236

matriv · 2018-11-09T01:30:16Z

retest this please

matriv · 2018-11-09T02:32:39Z

retest this please

costin

LGTM

astefan

LGTM. Looking great

Add `IsNull` node in parser to simplify expressions so that `<value> IS NULL` is no longer translated internally to `NOT(<value> IS NOT NULL)` Replace `IsNotNullProcessor` with `CheckNullProcessor` to encapsulate both isNull and isNotNull functionality. Closes: #34876 Fixes: #35171

matriv · 2018-11-09T10:51:23Z

Backported to 6.x with 771a940

Add `IsNull` node in parser to simplify expressions so that `<value> IS NULL` is no longer translated internally to `NOT(<value> IS NOT NULL)` Replace `IsNotNullProcessor` with `CheckNullProcessor` to encapsulate both isNull and isNotNull functionality. Closes: #34876 Fixes: #35171

matriv · 2018-11-09T11:11:49Z

Backported to 6.5 with 97adb4b

SQL: Introduce IsNull node to simplify expressions

b987100

Add `IsNull` node in parser to simplify expressions so that `<value> IS NULL` is no longer translated internally to `NOT(<value> IS NOT NULL)` Closes: elastic#34876

matriv added v7.0.0 :Analytics/SQL SQL querying v6.6.0 labels Nov 2, 2018

matriv requested review from costin and astefan November 2, 2018 13:16

Simplify by introducing negetable for UnaryScalarFunction

badd14f

matriv added the v6.5.1 label Nov 3, 2018

costin requested changes Nov 5, 2018

View reviewed changes

matriv added 2 commits November 6, 2018 23:24

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

f877db4

Address comments

f1069d8

costin reviewed Nov 7, 2018

View reviewed changes

matriv added 4 commits November 9, 2018 00:06

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

bf3f171

Address comments

6c7d3fa

Move FoldNull before constant folding

9d64843

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

658c625

costin approved these changes Nov 9, 2018

View reviewed changes

astefan approved these changes Nov 9, 2018

View reviewed changes

matriv merged commit 36da6e1 into elastic:master Nov 9, 2018

matriv deleted the mt/fix-34876 branch November 9, 2018 10:32

matriv added the >bug label Nov 20, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Introduce IsNull node to simplify expressions #35206

SQL: Introduce IsNull node to simplify expressions #35206

matriv commented Nov 2, 2018 •

edited

elasticmachine commented Nov 2, 2018

matriv commented Nov 2, 2018

matriv commented Nov 2, 2018

costin left a comment

costin Nov 5, 2018

astefan Nov 5, 2018

costin Nov 5, 2018

costin Nov 5, 2018

costin Nov 5, 2018

matriv Nov 5, 2018

costin Nov 7, 2018

costin Nov 5, 2018

costin Nov 5, 2018

matriv Nov 5, 2018

costin left a comment

costin Nov 7, 2018

matriv Nov 7, 2018 •

edited

costin Nov 7, 2018

costin Nov 7, 2018

matriv Nov 7, 2018

matriv commented Nov 9, 2018

matriv commented Nov 9, 2018

matriv commented Nov 9, 2018

costin left a comment

astefan left a comment

matriv commented Nov 9, 2018

matriv commented Nov 9, 2018


		import java.io.IOException;

		public class IsNullProcessor implements Processor {

		@@ -19,6 +19,10 @@

		public abstract class UnaryScalarFunction extends ScalarFunction {

		public interface UnaryNegateable {

		package org.elasticsearch.xpack.sql.expression.function.scalar;

		public interface Negateable {

SQL: Introduce IsNull node to simplify expressions #35206

SQL: Introduce IsNull node to simplify expressions #35206

Conversation

matriv commented Nov 2, 2018 • edited

elasticmachine commented Nov 2, 2018

matriv commented Nov 2, 2018

matriv commented Nov 2, 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

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv Nov 7, 2018 • edited

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 9, 2018

matriv commented Nov 9, 2018

matriv commented Nov 9, 2018

costin left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

matriv commented Nov 9, 2018

matriv commented Nov 9, 2018

matriv commented Nov 2, 2018 •

edited

matriv Nov 7, 2018 •

edited