SQL: Fix issue with wrong NULL optimization #37124

matriv · 2019-01-03T16:48:35Z

Logical operators OR and AND as well as conditional functions
(COALESCE, LEAST, GREATEST, etc.) cannot be folded to NULL if one
of their children is NULL as is the case for most of the functions.
Therefore, their nullable() implementation cannot return true. On
the other hand they cannot return false as if they're wrapped within
an IS NULL or IS NOT NULL expression, the expression will be folded
to false and true respectively leading to wrong results.

Change the signature of nullable() method and add a third value UKNOWN
to handle these cases.

Fixes: #35872

Logical operators `OR` and `AND` as well as conditional functions (`COALESCE`, `LEAST`, `GREATEST`, etc.) cannot be folded to NULL if one of their children is NULL as is the case for most of the functions. Therefore, their `nullable()` implementation cannot return `true`. On the other hand they cannot return `false` as if they're wrapped within an `IS NULL` or `IS NOT NULL` expression, the expression will be folded to `false` and `true` respectively leading to wrong results. Change the signature of `nullable()` method and add a third value `UKNOWN` to handle these cases. Fixes: elastic#35872

elasticmachine · 2019-01-03T16:48:37Z

Pinging @elastic/es-search

costin

Left some comments.
I think we're trying to address two issues at once:

figuring out whether something produces null or not
figure out if something supports null as a input

I'm not sure whether 1 and 2 need to be handled separately and in the meantime I think the current solution is sound. Especially since it affects older versions as well.

costin · 2019-01-03T20:38:39Z

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

@@ -29,6 +29,32 @@
 */
 public abstract class Expression extends Node<Expression> implements Resolvable {

+    public enum Nullable {


This is a good used enough enum that should be promoted to its own class - no need to keep it under Expression.
I wonder if Nullability is a better name - it avoid the clash with the Nullable annotation and also indicates a value of nullable as oppose to being an attribute by itself (or of itself).

+1 for Nullability

costin · 2019-01-03T20:40:21Z

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

@@ -29,6 +29,32 @@
 */
 public abstract class Expression extends Node<Expression> implements Resolvable {

+    public enum Nullable {
+        POSSIBLY,  // Whether the expression becomes null if at least one param/input is null


POSSIBLY is ambiguous as oppose to NEVER which is certain.
I would go for something simpler - TRUE, FALSE, UNKNOWN - shorter, clear and close to the SQL semantics.

Also the comment is inaccurate - null can be returned regardless of the input (there might not be any).

costin · 2019-01-03T20:42:22Z

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

+        NEVER,     // The expression can never become null
+        UNKNOWN;   // Cannot determine if the expression supports possible null folding
+
+        public static Nullable and(Nullable... args) {


There should one for or as well, no?

Not needed so far.

costin · 2019-01-03T21:01:32Z

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

+            // UKNOWN AND <anything> => UKNOWN
+            // NEVER AND NEVER => NEVER
+            // POSSIBLE AND NEVER/POSSIBLE => POSSIBLE
+            for (int i = 1; i < args.length; i++) {


The NEVER case doesn't seem to be handled. Also it might make sense to assign some bitmasks to the enum to make the comparison simpler and use just one accumulator.
That is, instead of checking the return value and the next value, it would be easier to just 'combine' the current value (returnValue) with the next value and be done with.
(an OO way would be to define a method on the enum but that adds a virtual call for minimum gain).
Further more since UNKNOWN trumps everything, the loop could be skipped if this value is found:

Nullability value = null; for (Nullability n: nullables) { switch (n) { case UNKNOWN: return UNKNOWN; case POSSIBLE: value = n; case NEVER: if (value == null) { value = n; } } } return value;

I like the bitmask idea!

matriv · 2019-01-03T22:19:55Z

@costin Addressed your comments. Could you please check again?

costin

Replied

costin · 2019-01-03T23:01:43Z

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

            }
        }
-        return false;
+        return Nullability.UNKNOWN;


false -> unknown? Does this fall under the "and" rule?

costin · 2019-01-03T23:02:06Z

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

-            DataType dataType, Literal literal) {
-        super(location, name, dataType, qualifier, nullable, id, synthetic);
+    public LiteralAttribute(Location location, String name, String qualifier, Nullability nullability, ExpressionId id, boolean synthetic,
+                            DataType dataType, Literal literal) {


Incorrect formatting?

why? It's the method signature and the args should align.

Because the signature is different than before; as far as I recall the alignment doesn't have to be under the same param (see the other constructors).

costin · 2019-01-03T23:03:18Z

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

+import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
+
+public enum Nullability {
+    TRUE((byte) 1),    // Whether the expression can become null


I'd remove the bitmask - it doesn't seem to add much value (see the previous method suggestions for implementing and); shorter and clearer than using bits.

matriv · 2019-01-03T23:49:49Z

@costin Thanks again, fixup pushed.

astefan · 2019-01-04T05:11:42Z

...gin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/regex/RegexMatch.java

-    public boolean nullable() {
-        return field().nullable() && pattern != null;
+    public Nullability nullable() {
+        if (pattern == null) {


I don't think the change here has the same outcome as the previous code?

yep, but before the logic was wrong as LIKE/RLIKE return null if the pattern is null or if the value checked is null.

astefan · 2019-01-04T05:12:16Z

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

@@ -1097,12 +1097,12 @@ private boolean canPropagateFoldable(LogicalPlan p) {
        @Override
        protected Expression rule(Expression e) {
            if (e instanceof IsNotNull) {
-                if (((IsNotNull) e).field().nullable() == false) {
+                if (((IsNotNull) e).field().nullable() == Nullability.FALSE) {


Can you static import this one?

Cannot, mixes up with Literal.TRUE and Literal.FALSE which are already statically imported.

astefan

LGTM. Left couple of comments.

astefan · 2019-01-04T08:24:26Z

I think the correct label for 6.6.x is 6.6.0.

costin

LGTM

costin · 2019-01-04T13:14:01Z

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

+                    value = TRUE;
+                    break;
+                case FALSE:
+                    if (value == null || value == FALSE) {


if (value == FALSE) value = FALSE; doesn't change anything so it can be removed.

costin · 2019-01-04T13:15:36Z

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

+                    }
+            }
+        }
+        return value != null ? value : UNKNOWN;


Does this ever occur? Is the method called over an empty list?

No it's a "safety net". Should we throw an exception instead?

That might make sense though my preference is to handle this corner cases leniently.
I was a bit confused by UNKNOWN, I would argue an empty list has FALSE nullability (it can never be null) but then again maybe it's something that's worth having a check.

Logical operators OR and AND as well as conditional functions (COALESCE, LEAST, GREATEST, etc.) cannot be folded to NULL if one of their children is NULL as is the case for most of the functions. Therefore, their nullable() implementation cannot return true. On the other hand they cannot return false as if they're wrapped within an IS NULL or IS NOT NULL expression, the expression will be folded to false and true respectively leading to wrong results. Change the signature of nullable() method and add a third value UKNOWN to handle these cases. Fixes: #35872

matriv · 2019-01-06T17:19:05Z

Backported to 6.x with a6e9e7c

Logical operators OR and AND as well as conditional functions (COALESCE, LEAST, GREATEST, etc.) cannot be folded to NULL if one of their children is NULL as is the case for most of the functions. Therefore, their nullable() implementation cannot return true. On the other hand they cannot return false as if they're wrapped within an IS NULL or IS NOT NULL expression, the expression will be folded to false and true respectively leading to wrong results. Change the signature of nullable() method and add a third value UKNOWN to handle these cases. Fixes: #35872

matriv · 2019-01-06T19:00:02Z

Backported to 6.6 with 88a3c84

Logical operators OR and AND as well as conditional functions (COALESCE, LEAST, GREATEST, etc.) cannot be folded to NULL if one of their children is NULL as is the case for most of the functions. Therefore, their nullable() implementation cannot return true. On the other hand they cannot return false as if they're wrapped within an IS NULL or IS NOT NULL expression, the expression will be folded to false and true respectively leading to wrong results. Change the signature of nullable() method and add a third value UKNOWN to handle these cases. Fixes: #35872

matriv · 2019-01-06T19:33:43Z

Backported to 6.5 with be638a3

matriv added v7.0.0 :Analytics/SQL SQL querying v6.7.0 v6.5.5 v6.6.1 labels Jan 3, 2019

matriv requested review from costin and astefan January 3, 2019 16:48

costin requested changes Jan 3, 2019

View reviewed changes

Address comments

15470a7

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

7a07d5e

matriv added the >bug label Jan 3, 2019

costin reviewed Jan 3, 2019

View reviewed changes

matriv added 2 commits January 4, 2019 01:34

Address comments

764b5cd

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

7bfeff9

astefan reviewed Jan 4, 2019

View reviewed changes

astefan approved these changes Jan 4, 2019

View reviewed changes

matriv added v6.6.0 and removed v6.6.1 labels Jan 4, 2019

costin approved these changes Jan 4, 2019

View reviewed changes

matriv added 2 commits January 4, 2019 18:31

Address comment

cd2c32f

Address comment

5e7fe98

matriv merged commit da3d8fb into elastic:master Jan 6, 2019

matriv deleted the mt/fix-35872 branch January 6, 2019 16:29

colings86 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: Fix issue with wrong NULL optimization #37124

SQL: Fix issue with wrong NULL optimization #37124

matriv commented Jan 3, 2019

elasticmachine commented Jan 3, 2019

costin left a comment

costin Jan 3, 2019

matriv Jan 3, 2019

costin Jan 3, 2019

costin Jan 3, 2019

costin Jan 3, 2019

matriv Jan 3, 2019

costin Jan 3, 2019

matriv Jan 3, 2019

matriv commented Jan 3, 2019

costin left a comment

costin Jan 3, 2019

costin Jan 3, 2019

matriv Jan 3, 2019

costin Jan 4, 2019

costin Jan 3, 2019

matriv commented Jan 3, 2019

astefan Jan 4, 2019

matriv Jan 4, 2019

astefan Jan 4, 2019

matriv Jan 4, 2019

astefan left a comment

astefan commented Jan 4, 2019

costin left a comment

costin Jan 4, 2019

costin Jan 4, 2019

matriv Jan 4, 2019 •

edited

costin Jan 4, 2019

matriv commented Jan 6, 2019

matriv commented Jan 6, 2019

matriv commented Jan 6, 2019

SQL: Fix issue with wrong NULL optimization #37124

SQL: Fix issue with wrong NULL optimization #37124

Conversation

matriv commented Jan 3, 2019

elasticmachine commented Jan 3, 2019

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

matriv commented Jan 3, 2019

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

matriv commented Jan 3, 2019

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

astefan commented Jan 4, 2019

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

matriv Jan 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Jan 6, 2019

matriv commented Jan 6, 2019

matriv commented Jan 6, 2019

matriv Jan 4, 2019 •

edited