[SPARK-17897] [SQL] Fixed IsNotNull Constraint Inference Rule #16067

gatorsmile · 2016-11-29T21:34:01Z

What changes were proposed in this pull request?

The constraints of an operator is the expressions that evaluate to true for all the rows produced. That means, the expression result should be neither false nor unknown (NULL). Thus, we can conclude that IsNotNull on all the constraints, which are generated by its own predicates or propagated from the children. The constraint can be a complex expression. For better usage of these constraints, we try to push down IsNotNull to the lowest-level expressions (i.e., Attribute). IsNotNull can be pushed through an expression when it is null intolerant. (When the input is NULL, the null-intolerant expression always evaluates to NULL.)

Below is the existing code we have for IsNotNull pushdown.

  private def scanNullIntolerantExpr(expr: Expression): Seq[Attribute] = expr match {
    case a: Attribute => Seq(a)
    case _: NullIntolerant | IsNotNull(_: NullIntolerant) =>
      expr.children.flatMap(scanNullIntolerantExpr)
    case _ => Seq.empty[Attribute]
  }

IsNotNull itself is not null-intolerant. It converts null to false. If the expression does not include any Not-like expression, it works; otherwise, it could generate a wrong result. This PR is to fix the above function by removing the IsNotNull from the inference. After the fix, when a constraint has a IsNotNull expression, we infer new attribute-specific IsNotNull constraints if and only if IsNotNull appears in the root.

Without the fix, the following test case will return empty.

val data = Seq[java.lang.Integer](1, null).toDF("key")
data.filter("not key is not null").show()

Before the fix, the optimized plan is like

== Optimized Logical Plan ==
Project [value#1 AS key#3]
+- Filter (isnotnull(value#1) && NOT isnotnull(value#1))
   +- LocalRelation [value#1]

After the fix, the optimized plan is like

== Optimized Logical Plan ==
Project [value#1 AS key#3]
+- Filter NOT isnotnull(value#1)
   +- LocalRelation [value#1]

How was this patch tested?

Added a test

dongjoon-hyun · 2016-11-30T00:04:14Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

      expr = "cast((_1 + _2) as boolean)", expectedNonNullableColumns = Seq("_1", "_2"))
  }

+  test("SPARK-17897: Attribute is not NullIntolerant") {


New test case name?

SparkQA · 2016-11-30T00:16:44Z

Test build #69350 has finished for PR 16067 at commit 0722ae5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-30T01:08:33Z

Test build #69354 has finished for PR 16067 at commit f693040.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2016-11-30T01:45:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala

+  /**
+   * Infer the Attribute-specific IsNotNull constraints from the null intolerant child expressions
+   * of constraints.
+   */


Can this infer IsNotNull(a), IsNotNull(b) from IsNotNull(a) && IsNotNull(b)?

It should be split before entering this function. Let me add a test case.

This change simply ignores all IsNotNulls which are not the top expression. The above case works because Filter splits it. But if the constraint looks like Cast(IsNotNull(a), Integer) == 1, we won't infer IsNotNull(a) from it, right?

Yes. After this PR, we do not support it. This is a pretty rare case, right?

gatorsmile · 2016-11-30T03:43:33Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/ConstraintPropagationSuite.scala

+        resolveColumn(tr, "a") === 1,
+        IsNotNull(resolveColumn(tr, "c")),
+        IsNotNull(resolveColumn(tr, "a")),
+        IsNotNull(resolveColumn(tr, "b")))))


The test case is added.

gatorsmile · 2016-11-30T03:44:24Z

Also cc @cloud-fan @hvanhovell @sameeragarwal @marmbrus @rxin

SparkQA · 2016-11-30T06:16:16Z

Test build #69377 has finished for PR 16067 at commit a835f80.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2016-11-30T06:46:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala

+   * of constraints.
+   */
+  private def inferIsNotNullConstraints(constraint: Expression): Seq[Expression] =
+    constraint match {


we don't need this case, I think it can be covered by the next case?

Yeah, my original idea is to do a fast stop. After rethinking it, it might be fine.

cloud-fan · 2016-11-30T06:48:08Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala

  /**
   * Recursively explores the expressions which are null intolerant and returns all attributes
   * in these expressions.
   */


shall we rename it to scanNullIntolerantAttribute?

cloud-fan · 2016-11-30T06:51:19Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala


+  test("SPARK-17897: Fixed IsNotNull Constraint Inference Rule") {
+    val data = Seq[java.lang.Integer](1, null).toDF("key")
+    checkAnswer(data.filter("not key is not null"), Row(null))


shall we use DataFrame API? i.e. data.filter(!$"key".isNotNull). The string version looks weird...

cloud-fan · 2016-11-30T06:51:51Z

LGTM except some minor comments

SparkQA · 2016-11-30T07:07:36Z

Test build #69388 has started for PR 16067 at commit 54c0dd1.

cloud-fan · 2016-11-30T08:25:01Z

retest this please

SparkQA · 2016-11-30T11:02:22Z

Test build #69396 has finished for PR 16067 at commit 54c0dd1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2016-11-30T11:41:33Z

thanks, merging to master/2.1!

### What changes were proposed in this pull request? The `constraints` of an operator is the expressions that evaluate to `true` for all the rows produced. That means, the expression result should be neither `false` nor `unknown` (NULL). Thus, we can conclude that `IsNotNull` on all the constraints, which are generated by its own predicates or propagated from the children. The constraint can be a complex expression. For better usage of these constraints, we try to push down `IsNotNull` to the lowest-level expressions (i.e., `Attribute`). `IsNotNull` can be pushed through an expression when it is null intolerant. (When the input is NULL, the null-intolerant expression always evaluates to NULL.) Below is the existing code we have for `IsNotNull` pushdown. ```Scala private def scanNullIntolerantExpr(expr: Expression): Seq[Attribute] = expr match { case a: Attribute => Seq(a) case _: NullIntolerant | IsNotNull(_: NullIntolerant) => expr.children.flatMap(scanNullIntolerantExpr) case _ => Seq.empty[Attribute] } ``` **`IsNotNull` itself is not null-intolerant.** It converts `null` to `false`. If the expression does not include any `Not`-like expression, it works; otherwise, it could generate a wrong result. This PR is to fix the above function by removing the `IsNotNull` from the inference. After the fix, when a constraint has a `IsNotNull` expression, we infer new attribute-specific `IsNotNull` constraints if and only if `IsNotNull` appears in the root. Without the fix, the following test case will return empty. ```Scala val data = Seq[java.lang.Integer](1, null).toDF("key") data.filter("not key is not null").show() ``` Before the fix, the optimized plan is like ``` == Optimized Logical Plan == Project [value#1 AS key#3] +- Filter (isnotnull(value#1) && NOT isnotnull(value#1)) +- LocalRelation [value#1] ``` After the fix, the optimized plan is like ``` == Optimized Logical Plan == Project [value#1 AS key#3] +- Filter NOT isnotnull(value#1) +- LocalRelation [value#1] ``` ### How was this patch tested? Added a test Author: gatorsmile <gatorsmile@gmail.com> Closes #16067 from gatorsmile/isNotNull2. (cherry picked from commit 2eb093d) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

### What changes were proposed in this pull request? The `constraints` of an operator is the expressions that evaluate to `true` for all the rows produced. That means, the expression result should be neither `false` nor `unknown` (NULL). Thus, we can conclude that `IsNotNull` on all the constraints, which are generated by its own predicates or propagated from the children. The constraint can be a complex expression. For better usage of these constraints, we try to push down `IsNotNull` to the lowest-level expressions (i.e., `Attribute`). `IsNotNull` can be pushed through an expression when it is null intolerant. (When the input is NULL, the null-intolerant expression always evaluates to NULL.) Below is the existing code we have for `IsNotNull` pushdown. ```Scala private def scanNullIntolerantExpr(expr: Expression): Seq[Attribute] = expr match { case a: Attribute => Seq(a) case _: NullIntolerant | IsNotNull(_: NullIntolerant) => expr.children.flatMap(scanNullIntolerantExpr) case _ => Seq.empty[Attribute] } ``` **`IsNotNull` itself is not null-intolerant.** It converts `null` to `false`. If the expression does not include any `Not`-like expression, it works; otherwise, it could generate a wrong result. This PR is to fix the above function by removing the `IsNotNull` from the inference. After the fix, when a constraint has a `IsNotNull` expression, we infer new attribute-specific `IsNotNull` constraints if and only if `IsNotNull` appears in the root. Without the fix, the following test case will return empty. ```Scala val data = Seq[java.lang.Integer](1, null).toDF("key") data.filter("not key is not null").show() ``` Before the fix, the optimized plan is like ``` == Optimized Logical Plan == Project [value#1 AS key#3] +- Filter (isnotnull(value#1) && NOT isnotnull(value#1)) +- LocalRelation [value#1] ``` After the fix, the optimized plan is like ``` == Optimized Logical Plan == Project [value#1 AS key#3] +- Filter NOT isnotnull(value#1) +- LocalRelation [value#1] ``` ### How was this patch tested? Added a test Author: gatorsmile <gatorsmile@gmail.com> Closes apache#16067 from gatorsmile/isNotNull2.

yhuai · 2017-02-10T23:15:32Z

@gatorsmile can we also add it in branch-2.0? Thanks!

gatorsmile · 2017-02-11T06:57:27Z

Sure, let me back port it now.

… Rule ### What changes were proposed in this pull request? This PR is to backport #16067 to Spark 2.0 ---- The `constraints` of an operator is the expressions that evaluate to `true` for all the rows produced. That means, the expression result should be neither `false` nor `unknown` (NULL). Thus, we can conclude that `IsNotNull` on all the constraints, which are generated by its own predicates or propagated from the children. The constraint can be a complex expression. For better usage of these constraints, we try to push down `IsNotNull` to the lowest-level expressions (i.e., `Attribute`). `IsNotNull` can be pushed through an expression when it is null intolerant. (When the input is NULL, the null-intolerant expression always evaluates to NULL.) Below is the existing code we have for `IsNotNull` pushdown. ```Scala private def scanNullIntolerantExpr(expr: Expression): Seq[Attribute] = expr match { case a: Attribute => Seq(a) case _: NullIntolerant | IsNotNull(_: NullIntolerant) => expr.children.flatMap(scanNullIntolerantExpr) case _ => Seq.empty[Attribute] } ``` **`IsNotNull` itself is not null-intolerant.** It converts `null` to `false`. If the expression does not include any `Not`-like expression, it works; otherwise, it could generate a wrong result. This PR is to fix the above function by removing the `IsNotNull` from the inference. After the fix, when a constraint has a `IsNotNull` expression, we infer new attribute-specific `IsNotNull` constraints if and only if `IsNotNull` appears in the root. Without the fix, the following test case will return empty. ```Scala val data = Seq[java.lang.Integer](1, null).toDF("key") data.filter("not key is not null").show() ``` Before the fix, the optimized plan is like ``` == Optimized Logical Plan == Project [value#1 AS key#3] +- Filter (isnotnull(value#1) && NOT isnotnull(value#1)) +- LocalRelation [value#1] ``` After the fix, the optimized plan is like ``` == Optimized Logical Plan == Project [value#1 AS key#3] +- Filter NOT isnotnull(value#1) +- LocalRelation [value#1] ``` ### How was this patch tested? Added a test Author: Xiao Li <gatorsmile@gmail.com> Closes #16894 from gatorsmile/isNotNull2.0.

gatorsmile added 3 commits November 29, 2016 00:00

fix.

33c10a0

fix.

025632a

improve the comments.

0722ae5

gatorsmile changed the title ~~[SPARK-17897] [SQL] Fixed IsNotNull Inference Rule~~ [SPARK-17897] [SQL] Fixed IsNotNull Constraint Inference Rule Nov 29, 2016

rename the func

f693040

dongjoon-hyun reviewed Nov 30, 2016

View reviewed changes

viirya reviewed Nov 30, 2016

View reviewed changes

address comments.

a835f80

gatorsmile commented Nov 30, 2016

View reviewed changes

cloud-fan reviewed Nov 30, 2016

View reviewed changes

address comments.

54c0dd1

asfgit closed this in 2eb093d Nov 30, 2016

gatorsmile mentioned this pull request Feb 11, 2017

[SPARK-17897] [SQL] [BACKPORT-2.0] Fixed IsNotNull Constraint Inference Rule #16894

Closed

JoshRosen mentioned this pull request Jun 1, 2019

[SPARK-27915][SQL][WIP] Update logical Filter's output nullability based on IsNotNull conditions #24765

Closed

[SPARK-17897] [SQL] Fixed IsNotNull Constraint Inference Rule #16067

[SPARK-17897] [SQL] Fixed IsNotNull Constraint Inference Rule #16067

Uh oh!

Conversation

gatorsmile commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 30, 2016

Uh oh!

SparkQA commented Nov 30, 2016

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!

gatorsmile commented Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 30, 2016

Uh oh!

cloud-fan Nov 30, 2016 • 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!

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!

cloud-fan commented Nov 30, 2016

Uh oh!

SparkQA commented Nov 30, 2016

Uh oh!

cloud-fan commented Nov 30, 2016

Uh oh!

SparkQA commented Nov 30, 2016

Uh oh!

cloud-fan commented Nov 30, 2016

Uh oh!

yhuai commented Feb 10, 2017

Uh oh!

gatorsmile commented Feb 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gatorsmile commented Nov 29, 2016 •

edited

Loading

gatorsmile commented Nov 30, 2016 •

edited

Loading

cloud-fan Nov 30, 2016 •

edited

Loading