[SPARK-29162][SQL] Simplify NOT(IsNull(x)) and NOT(IsNotNull(x)) #25878

AngersZhuuuu · 2019-09-21T04:23:29Z

What changes were proposed in this pull request?

Rewrite

NOT isnull(x)     -> isnotnull(x)
NOT isnotnull(x)  -> isnull(x)

Why are the changes needed?

Make LogicalPlan more readable and useful for query canonicalization. Make same condition equal when judge query canonicalization equal

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Newly added UTs.

AngersZhuuuu · 2019-09-21T04:27:13Z

gentle ping @juliuszsompolski
I agree a lot that we should simplify these expression. It's really useful for judge query canonicalization equal. Meet this problem when I am doing some cache framework .

maropu · 2019-09-21T07:55:06Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala

        Literal.create(null, e.dataType)
+
+      case n@Not(expr: IsNull) => IsNotNull(expr.child)
+      case n@Not(expr: IsNotNull) => IsNull(expr.child)


Why do you handle this in NullPropagation?

Also, plz check the code style carefully?

Also, plz check the code style carefully?

Sorry, forget to check this place...

Why do you handle this in NullPropagation?

Don't found other better place, and it's also NULL expression problem.
Or add a new class?

Yea, the @viirya suggestion looks nice to me.

HyukjinKwon · 2019-09-21T15:16:38Z

ok to test

SparkQA · 2019-09-21T19:12:23Z

Test build #111122 has finished for PR 25878 at commit 9bf2d7c.

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

viirya · 2019-09-21T19:50:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala

        }
+
+      case n @ Not(expr: IsNull) => IsNotNull(expr.child)
+      case n @ Not(expr: IsNotNull) => IsNull(expr.child)


you don't need n.

viirya · 2019-09-21T19:50:55Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/SimplifyConditionalSuite.scala

  }

+  private def assertFilter(originalExpr: Expression,
+                           expectedExpr: Expression): Unit = {


indent is wrong.

viirya · 2019-09-21T19:58:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala

          e.copy(branches = branches.take(i).map(branch => (branch._1, elseValue)))
        }
+
+      case n @ Not(expr: IsNull) => IsNotNull(expr.child)


IsNull and IsNotNull are conditional expressions? Seems BooleanSimplification is more suitable.

conditional

Reasonable, I focus too much on isnull/isnotnull expression。

maropu · 2019-09-22T00:36:05Z

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

    }
  }
+
+  test("SPARK-29152: Simplify NOT(IsNull(x)) and NOT(IsNotNull(x))") {


nit: I personally think the SimplifyConditionalSuite test is enough for this fix.

nit: I personally think the SimplifyConditionalSuite test is enough for this fix.

Yeah, seems end-to-end test in SQLQuerySuit is redundant

maropu · 2019-09-22T00:39:21Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/SimplifyConditionalSuite.scala

  }
+
+  test("simplify NOT(IsNull(x)) and NOT(IsNotNull(x))") {
+    assertFilter(Not(IsNotNull(UnresolvedAttribute("b"))), IsNull(UnresolvedAttribute("b")))


We cannot use assertEquivalent here?

We cannot use assertEquivalent here?

assertEquivalent use OneRowRelation, no column, so I add a testRelation.
I will remove it to BooleanSimplificationSuit .

maropu

LGTM @HyukjinKwon @viirya

dongjoon-hyun · 2019-09-22T03:54:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala

      case Not(Not(e)) => e
+
+      case Not(expr: IsNull) => IsNotNull(expr.child)
+      case Not(expr: IsNotNull) => IsNull(expr.child)


nit. expr -> e.

case Not(e: IsNull) => IsNotNull(e.child) case Not(e: IsNotNull) => IsNull(e.child)

nit. expr -> e.

How about

case Not(IsNull(e)) => IsNotNull(e) case Not(IsNotNull(e)) => IsNull(e)

dongjoon-hyun · 2019-09-22T04:02:09Z

...lyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala

+  test("simplify NOT(IsNull(x)) and NOT(IsNotNull(x))") {
+    checkCondition(Not(IsNotNull(UnresolvedAttribute("b"))), IsNull(UnresolvedAttribute("b")))
+    checkCondition(Not(IsNull(UnresolvedAttribute("b"))), IsNotNull(UnresolvedAttribute("b")))
+  }


@AngersZhuuuu . Since you are active contribution, I'd like to recommend to use DSL as possible as you can. (Please refer the other tests.)

test("simplify NOT(IsNull(x)) and NOT(IsNotNull(x))") { checkCondition(Not(IsNotNull('e)), IsNull('e)) checkCondition(Not(IsNull('e)), IsNotNull('e)) }

@AngersZhuuuu . Since you are active contribution, I'd like to recommend to use DSL as possible as you can. (Please refer the other tests.)

First time use DSL, I will get to know DSL more

dongjoon-hyun · 2019-09-22T04:03:08Z

@AngersZhuuuu . Thank you for your contributions. I left two minor comments. Please address them.

AngersZhuuuu · 2019-09-22T04:10:36Z

@AngersZhuuuu . Thank you for your contributions. I left two minor comments. Please address them.

Thank you for your advise. All for better job.

SparkQA · 2019-09-22T04:16:37Z

Test build #111131 has finished for PR 25878 at commit a75c2ef.

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

SparkQA · 2019-09-22T05:09:14Z

Test build #111133 has finished for PR 25878 at commit cb9cef2.

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

SparkQA · 2019-09-22T07:05:02Z

Test build #111142 has finished for PR 25878 at commit f71698d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

AngersZhuuuu · 2019-09-22T12:51:38Z

@AngersZhuuuu . Thank you for your contributions. I left two minor comments. Please address them.

Can you trigger restart test. Thanks .

maropu · 2019-09-22T13:18:26Z

retest this please

SparkQA · 2019-09-22T17:18:02Z

Test build #111156 has finished for PR 25878 at commit f71698d.

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

dongjoon-hyun

+1, LGTM. Merged to master. Thank you all!

Simplify NOT(isnull(x)) and NOT(isnotnull(x))

fa8b240

dongjoon-hyun added the SQL label Sep 21, 2019

maropu reviewed Sep 21, 2019

View reviewed changes

AngersZhuuuu added 4 commits September 21, 2019 16:22

fix code style

a701dca

move optimize place

978a284

change test method

f139558

remove unused import

9bf2d7c

viirya reviewed Sep 21, 2019

View reviewed changes

follow comment

a75c2ef

maropu reviewed Sep 22, 2019

View reviewed changes

AngersZhuuuu added 2 commits September 22, 2019 08:52

fix UT place

1fdea48

remove unused import

cb9cef2

maropu approved these changes Sep 22, 2019

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-29162][SQL]Simplify NOT(IsNull(x)) and NOT(IsNotNull(x))~~ [SPARK-29162][SQL] Simplify NOT(IsNull(x)) and NOT(IsNotNull(x)) Sep 22, 2019

dongjoon-hyun reviewed Sep 22, 2019

View reviewed changes

follow comment

9f5defa

dongjoon-hyun reviewed Sep 22, 2019

View reviewed changes

for better code style

f71698d

viirya approved these changes Sep 22, 2019

View reviewed changes

dongjoon-hyun approved these changes Sep 22, 2019

View reviewed changes

dongjoon-hyun closed this in fe4bee8 Sep 22, 2019

peter-toth mentioned this pull request Mar 21, 2021

[SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function #31913

Closed

[SPARK-29162][SQL] Simplify NOT(IsNull(x)) and NOT(IsNotNull(x)) #25878

[SPARK-29162][SQL] Simplify NOT(IsNull(x)) and NOT(IsNotNull(x)) #25878

Uh oh!

Conversation

AngersZhuuuu commented Sep 21, 2019 • edited by dongjoon-hyun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

AngersZhuuuu commented Sep 21, 2019

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!

HyukjinKwon commented Sep 21, 2019

Uh oh!

SparkQA commented Sep 21, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu 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!

dongjoon-hyun commented Sep 22, 2019

Uh oh!

AngersZhuuuu commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

AngersZhuuuu commented Sep 22, 2019

Uh oh!

maropu commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

AngersZhuuuu commented Sep 21, 2019 •

edited by dongjoon-hyun

Loading