[SPARK-30353][SQL] Add IsNotNull check in SimplifyBinaryComparison optimization #27008

ulysses-you · 2019-12-25T13:51:30Z

What changes were proposed in this pull request?

Now Spark can propagate constraint during sql optimization when spark.sql.constraintPropagation.enabled is true, then where c = 1 will convert to where c = 1 and c is not null. We also can use constraint in SimplifyBinaryComparison.

SimplifyBinaryComparison will simplify expression which is not nullable and semanticEquals. And we also can simplify if one expression is infered IsNotNull.

Why are the changes needed?

Simplify SQL.

create table test (c1 string);

explain extended select c1 from test where c1 = c1 limit 10;
-- before
GlobalLimit 10
+- LocalLimit 10
   +- Filter (isnotnull(c1#20) AND (c1#20 = c1#20))
      +- Relation[c1#20]
-- after
GlobalLimit 10
+- LocalLimit 10
    +- Filter (isnotnull(c1#20)
        +- Relation[c1#20]

explain extended select c1 from test where c1 > c1 limit 10;
-- before
GlobalLimit 10
+- LocalLimit 10
   +- Filter (isnotnull(c1#20) && (c1#20 > c1#20))
      +- Relation[c1#20]
-- after
LocalRelation <empty>, [c1#20]

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add UT.

SparkQA · 2019-12-25T15:04:40Z

Test build #115772 has finished for PR 27008 at commit 3024dd8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

ulysses-you · 2019-12-26T00:38:19Z

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

@@ -115,6 +114,7 @@ abstract class Optimizer(catalogManager: CatalogManager)
      Batch("Infer Filters", Once,
        InferFiltersFromConstraints) ::
      Batch("Operator Optimization after Inferring Filters", fixedPoint,
+        Seq(SimplifyBinaryComparison) ++


We should apply InferFiltersFromConstraints first and then apply SimplifyBinaryComparison to retain isnotnull filter.

I think the rules in operatorOptimizationRuleSet are run after inferring filters and SimplifyBinaryComparison is in the rules, no?

Because operatorOptimizationRuleSet will run double time. Once before and once after InferFiltersFromConstraints.

We should only do thie optimization after InferFiltersFromConstraints.

SparkQA · 2019-12-26T04:38:28Z

Test build #115779 has finished for PR 27008 at commit 0da71f8.

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

maropu · 2019-12-26T12:21:49Z

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

+    }
+
+    if (SQLConf.get.constraintPropagationEnabled) {
+      canSimplify(left, right) || plan.constraints.exists {


Can we limit this constrain check only in Filter plans?

I think yes, this rely on Filter constraint which InferFiltersFromConstraints add.

maropu · 2019-12-26T12:24:08Z

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

-      case a GreaterThan b if !a.nullable && !b.nullable && a.semanticEquals(b) => FalseLiteral
-      case a LessThan b if !a.nullable && !b.nullable && a.semanticEquals(b) => FalseLiteral
+      case a GreaterThan b if canSimplifyWithConstraints(l, a, b) => FalseLiteral
+      case a LessThan b if canSimplifyWithConstraints(l, a, b) => FalseLiteral


canSimplifyWithConstraints => canSimplifyComparison?

maropu · 2019-12-26T12:24:21Z

cc: @viirya

SparkQA · 2019-12-26T18:22:20Z

Test build #115815 has finished for PR 27008 at commit 79c97ff.

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

viirya · 2019-12-26T19:26:29Z

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

+
+  private def canSimplifyComparison(
+      plan: LogicalPlan, left: Expression, right: Expression): Boolean = {
+    def canSimplify(left: Expression, right: Expression): Boolean = {


canSimplify is not reuse. We can just inline it. It will looks more clear.

viirya · 2019-12-26T19:47:21Z

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

+      return true
+    }
+
+    if (SQLConf.get.constraintPropagationEnabled) {


Is this optimization constraint propagation only? When constraintPropagationEnabled is disabled, a Filter with predicate like IsNotNull(e) && e > e can still be simplified, no?

Good catch! It also can optimize suchIsNotNull(e) && e > e without constraintPropagationEnabled.

ulysses-you · 2019-12-27T01:55:18Z

We can make this simply.

We just need to check if IsNotNull exists. If exists then optimize else not. Constraints is used by InferFiltersFromConstraints, we don't need to rely on it strongly.

I changed the pr title.
cc @maropu @viirya

HyukjinKwon · 2019-12-27T03:56:38Z

...test/scala/org/apache/spark/sql/catalyst/optimizer/BinaryComparisonSimplificationSuite.scala

@@ -122,4 +127,49 @@ class BinaryComparisonSimplificationSuite extends PlanTest with PredicateHelper

    comparePlans(optimized, correctAnswer)
  }
+
+  test("Simplify null and nonnull with filter constraints") {
+    Seq('a === 'a, 'a <= 'a, 'a >= 'a, 'a < 'a, 'a > 'a).foreach { condition =>


nit: can't we avoid to use symbol? See https://github.com/databricks/scala-style-guide#symbol

Thanks for the remind. Is there any simply way to instead of symbol ?

At the very least you can do Symbol(...). This isn't a nit actually because we should support Scala 2.13 properly.

Oh I see. The scala-style-guide has the new update. Thanks again.

SparkQA · 2019-12-27T05:20:19Z

Test build #115827 has finished for PR 27008 at commit 984e607.

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

SparkQA · 2019-12-27T06:24:40Z

Test build #115828 has finished for PR 27008 at commit 2c4dd63.

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

SparkQA · 2019-12-27T08:05:02Z

Test build #115846 has finished for PR 27008 at commit 9b0d268.

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

maropu · 2019-12-28T05:50:13Z

retest this please

maropu · 2019-12-28T08:01:16Z

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

@@ -384,22 +384,40 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper {
 /**
 * Simplifies binary comparisons with semantically-equal expressions:
 * 1) Replace '<=>' with 'true' literal.
- * 2) Replace '=', '<=', and '>=' with 'true' literal if both operands are non-nullable.
- * 3) Replace '<' and '>' with 'false' literal if both operands are non-nullable.


We need to update these statements above? IIUC this pr just checks more for non-nullable cases?

Thanks for remind, seems not need. Revert this.

SparkQA · 2019-12-28T08:05:02Z

Test build #115883 has finished for PR 27008 at commit 9b0d268.

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

maropu · 2019-12-28T08:11:22Z

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

+        }
+
+      case _ => false
+    }


How about this?

private def canSimplifyComparison( plan: LogicalPlan, left: Expression, right: Expression): Boolean = { if (!left.nullable && !right.nullable && left.semanticEquals(right)) { true } else { // We do more checks for non-nullable cases plan match { case Filter(fc, _) => splitConjunctivePredicates(fc).exists { condition => condition.semanticEquals(IsNotNull(left)) && condition.semanticEquals(IsNotNull(right)) } case _ => false } } }

Looks fine.

SparkQA · 2019-12-30T04:10:58Z

Test build #115922 has finished for PR 27008 at commit 87d1c6c.

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

SparkQA · 2019-12-30T04:34:17Z

Test build #115923 has finished for PR 27008 at commit 1c1630e.

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

ulysses-you · 2020-01-09T03:46:02Z

@maropu @HyukjinKwon @viirya Do you have time to review again? Any feedback is great.
Also cc @cloud-fan .

cloud-fan · 2020-01-09T12:46:09Z

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

+      plan match {
+        case Filter(fc, _) =>
+          splitConjunctivePredicates(fc).exists { condition =>
+            condition.semanticEquals(IsNotNull(left)) && condition.semanticEquals(IsNotNull(right))


this is always false IIUC. How can a condition be both IsNotNull(left) and IsNotNull(right)?

In this scene, left is semanticEquals to right. Like this where c > c, and if there exists IsNotNull(c), then will return true.

then how about

if (left.semanticEquals(right)) { if (!left.nullable && !right.nullable) { true } else { plan match ... condition.semanticEquals(IsNotNull(left)) } } else { false }

Make sense.

SparkQA · 2020-01-10T12:48:11Z

Test build #116473 has finished for PR 27008 at commit af0fbd6.

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

cloud-fan · 2020-01-10T12:57:23Z

...test/scala/org/apache/spark/sql/catalyst/optimizer/BinaryComparisonSimplificationSuite.scala

+        .foreach { condition =>
+        val plan = nullableRelation.where(condition).analyze
+        val actual = Optimize.execute(plan)
+        val correctAnswer = nullableRelation.analyze


shouldn't this be nullableRelation.where(false)?

False has been remove by PruneFilters, so result is just empty LocalRelation.

cloud-fan · 2020-01-12T07:04:09Z

thanks, merging to master!

ulysses-you · 2020-01-12T07:35:33Z

@cloud-fan thanks for merging. Thanks all !

…me complexity ### What changes were proposed in this pull request? The changes in the rule `SimplifyBinaryComparison` from #27008 could bring performance regression in the optimizer when there are a large set of filter conditions. We need to improve the implementation and reduce the time complexity. ### Why are the changes needed? Need to fix the potential performance regression in the optimizer. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Existing unit tests. Also run a micor benchmark in `BinaryComparisonSimplificationSuite` ``` object Optimize extends RuleExecutor[LogicalPlan] { val batches = Batch("Constant Folding", FixedPoint(50), SimplifyBinaryComparison) :: Nil } test("benchmark") { val a = Symbol("a") val condition = (1 to 500).map(i => EqualTo(a, a)).reduceLeft(And) val finalCondition = And(condition, IsNotNull(a)) val plan = nullableRelation.where(finalCondition).analyze val start = System.nanoTime() Optimize.execute(plan) println((System.nanoTime() - start) /1000000) } ``` Before the changes: 2507ms After the changes: 3ms Closes #27212 from gengliangwang/SimplifyBinaryComparison. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

init

3024dd8

fix optimization order

0da71f8

ulysses-you commented Dec 26, 2019

View reviewed changes

maropu reviewed Dec 26, 2019

View reviewed changes

limit filter constraint

79c97ff

viirya reviewed Dec 26, 2019

View reviewed changes

ulysses-you added 2 commits December 27, 2019 09:14

optimize without constraint

984e607

make optimization easy

2c4dd63

ulysses-you changed the title ~~[SPARK-30353][SQL] Use constraints in SimplifyBinaryComparison optimization~~ [SPARK-30353][SQL] Add IsNotNull check in SimplifyBinaryComparison optimization Dec 27, 2019

HyukjinKwon reviewed Dec 27, 2019

View reviewed changes

avoid symbol

9b0d268

maropu reviewed Dec 28, 2019

View reviewed changes

ulysses-you added 2 commits December 30, 2019 08:15

fix

87d1c6c

add comment

1c1630e

cloud-fan reviewed Jan 9, 2020

View reviewed changes

fix

af0fbd6

cloud-fan reviewed Jan 10, 2020

View reviewed changes

cloud-fan closed this in 823e3d3 Jan 12, 2020

gengliangwang mentioned this pull request Jan 15, 2020

[SPARK-30515][SQL] Refactor SimplifyBinaryComparison to reduce the time complexity #27212

Closed

dongjoon-hyun added the SQL label Feb 5, 2020

ulysses-you deleted the SPARK-30353 branch July 4, 2022 10:00

[SPARK-30353][SQL] Add IsNotNull check in SimplifyBinaryComparison optimization #27008

[SPARK-30353][SQL] Add IsNotNull check in SimplifyBinaryComparison optimization #27008

Conversation

ulysses-you commented Dec 25, 2019 • edited

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?

SparkQA commented Dec 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulysses-you Dec 27, 2019 • edited

Choose a reason for hiding this comment

SparkQA commented Dec 26, 2019

Choose a reason for hiding this comment

ulysses-you Dec 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu commented Dec 26, 2019

SparkQA commented Dec 26, 2019

viirya Dec 26, 2019 • 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

ulysses-you commented Dec 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon Dec 27, 2019 • edited

Choose a reason for hiding this comment

ulysses-you Dec 27, 2019 • edited

Choose a reason for hiding this comment

SparkQA commented Dec 27, 2019

SparkQA commented Dec 27, 2019

SparkQA commented Dec 27, 2019

maropu commented Dec 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 30, 2019

SparkQA commented Dec 30, 2019

ulysses-you commented Jan 9, 2020

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

SparkQA commented Jan 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jan 12, 2020

ulysses-you commented Jan 12, 2020

ulysses-you commented Dec 25, 2019 •

edited

ulysses-you Dec 27, 2019 •

edited

ulysses-you Dec 26, 2019 •

edited

viirya Dec 26, 2019 •

edited

ulysses-you commented Dec 27, 2019 •

edited

HyukjinKwon Dec 27, 2019 •

edited

ulysses-you Dec 27, 2019 •

edited