[SPARK-22719][SQL]Refactor ConstantPropagation #19912

gengliangwang · 2017-12-06T16:56:55Z

What changes were proposed in this pull request?

The current time complexity of ConstantPropagation is O(n^2), which can be slow when the query is complex.
Refactor the implementation with O( n ) time complexity, and some pruning to avoid traversing the whole Condition

How was this patch tested?

Unit test.

Also simple benchmark test in ConstantPropagationSuite

  val condition = (1 to 500).map{_ => Rand(0) === Rand(0)}.reduce(And)
  val query = testRelation
    .select(columnA)
    .where(condition)
  val start = System.currentTimeMillis()
  (1 to 40).foreach { _ =>
    Optimize.execute(query.analyze)
  }
  val end = System.currentTimeMillis()
  println(end - start)

Run time before changes: 18989ms (474ms per loop)
Run time after changes: 1275 ms (32ms per loop)

SparkQA · 2017-12-06T19:43:28Z

Test build #84566 has finished for PR 19912 at commit 962faab.

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

gatorsmile · 2017-12-06T18:35:03Z

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

+        (None, Seq(((right, left), e)))
+      case a: And =>
+        val (newLeft, equalityPredicatesLeft) = traverse(a.left, false)
+        val (newRight, equalityPredicatesRight) = traverse(a.right, false)


replaceChildren = false

gatorsmile · 2017-12-06T18:36:01Z

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

-            }
+  /**
+   * Traverse a condition as a tree and replace attributes with constant values.
+   * - If the child of [[And]] is [[EqualTo]] or [[EqualNullSafe]], propagate the mapping


the child -> any child

gatorsmile · 2017-12-06T19:12:22Z

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

+        } else {
+          None
+        }
+        (newSelf, Seq.empty)


val (newChild, _) = traverse(n.child, replaceChildren = true) (newChild.map(Not), Seq.empty)

gatorsmile · 2017-12-06T19:41:44Z

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

+        (newSelf, equalityPredicates)
+      case o: Or =>
+        val (newLeft, _) = traverse(o.left, true)
+        val (newRight, _) = traverse(o.right, true)


The same here.

gatorsmile · 2017-12-06T19:43:51Z

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

+        }
+        (newSelf, equalityPredicates)
+      case o: Or =>
+        val (newLeft, _) = traverse(o.left, true)


Add comments to explains why we do not need to consider the returned equalityPredicates

gatorsmile · 2017-12-06T19:44:05Z

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

        }
+        (newSelf, Seq.empty)
+      case n: Not =>
+        val (newChild, _) = traverse(n.child, true)


Also here. add a comment.

gatorsmile · 2017-12-06T19:58:10Z

Overall, it looks good to me. cc @jiangxb1987 @cloud-fan

viirya · 2017-12-07T01:45:22Z

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

+   *   attributes with the corresponding constant values in both children with propagated mapping.
+   * @param condition condition to be traversed
+   * @param replaceChildren whether to replace attributes with the corresponding constant values
+   */


Add doc to explain return value?

viirya · 2017-12-07T01:55:24Z

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

+   * - If the child of [[And]] is [[EqualTo]] or [[EqualNullSafe]], propagate the mapping
+   *   of attribute => constant.
+   * - If the current [[And]] node is not child of another [[And]], replace occurrence of the
+   *   attributes with the corresponding constant values in both children with propagated mapping.


Also add the comments to explain Or and Not processing below.

viirya · 2017-12-07T02:01:22Z

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

  def apply(plan: LogicalPlan): LogicalPlan = plan transform {
-    case f: Filter => f transformExpressionsUp {


Will it be the same effect if we turn transformExpressionsUp to transformExpressionsDown?

Oh, this doesn't work because it still replaces the replaced And.

viirya · 2017-12-07T02:14:14Z

New algorithm looks correct and good.

viirya · 2017-12-07T02:16:55Z

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

+   * @param replaceChildren whether to replace attributes with the corresponding constant values
+   */
+  private def traverse(condition: Expression, replaceChildren: Boolean)
+    : (Option[Expression], Seq[((AttributeReference, Literal), BinaryComparison)]) =


Maybe define a type alias for Seq[((AttributeReference, Literal), BinaryComparison)].

gengliangwang · 2017-12-07T05:31:17Z

@gatorsmile @viirya thanks for reviewing! I have addressed your comments.

SparkQA · 2017-12-07T06:56:18Z

Test build #84593 has finished for PR 19912 at commit 88cd025.

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

jiangxb1987 · 2017-12-07T07:56:09Z

retest this please

SparkQA · 2017-12-07T08:05:02Z

Test build #84596 has finished for PR 19912 at commit 88cd025.

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

viirya · 2017-12-07T08:06:45Z

retest this please.

jiangxb1987

LGTM

SparkQA · 2017-12-07T10:56:42Z

Test build #84598 has finished for PR 19912 at commit 88cd025.

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

viirya · 2017-12-07T13:10:33Z

LGTM

cloud-fan · 2017-12-07T14:53:30Z

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

+    : Expression = {
+    val constantsMap = AttributeMap(equalityPredicates.map(_._1))
+    val predicates = equalityPredicates.map(_._2).toSet
+    def _replaceConstants(expression: Expression) = expression transform {


nit: replaceConstants0 is more java style

cloud-fan · 2017-12-07T14:57:03Z

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

+    val predicates = equalityPredicates.map(_._2).toSet
+    def _replaceConstants(expression: Expression) = expression transform {
+      case a: AttributeReference =>
+        constantsMap.get(a) match {


constentsMap.getOrElse(a, a)

cloud-fan · 2017-12-07T15:04:29Z

LGTM except 2 minor style comments

SparkQA · 2017-12-07T18:20:09Z

Test build #84605 has finished for PR 19912 at commit 433587a.

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

gatorsmile · 2017-12-07T18:24:45Z

Thanks! Merged to master.

refactor ConstantPropagation

962faab

gatorsmile reviewed Dec 6, 2017

View reviewed changes

viirya reviewed Dec 7, 2017

View reviewed changes

address comments

88cd025

jiangxb1987 approved these changes Dec 7, 2017

View reviewed changes

cloud-fan reviewed Dec 7, 2017

View reviewed changes

address Wenchen's comments

433587a

asfgit closed this in 18b75d4 Dec 7, 2017

peter-toth mentioned this pull request Apr 30, 2019

[SPARK-27604][SQL] Add filter reduction #24495

Closed

		def apply(plan: LogicalPlan): LogicalPlan = plan transform {
		case f: Filter => f transformExpressionsUp {

[SPARK-22719][SQL]Refactor ConstantPropagation #19912

[SPARK-22719][SQL]Refactor ConstantPropagation #19912

Uh oh!

Conversation

gengliangwang commented Dec 6, 2017 • 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!

SparkQA commented Dec 6, 2017

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 Dec 6, 2017

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!

viirya commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengliangwang commented Dec 7, 2017

Uh oh!

SparkQA commented Dec 7, 2017

Uh oh!

jiangxb1987 commented Dec 7, 2017

Uh oh!

SparkQA commented Dec 7, 2017

Uh oh!

viirya commented Dec 7, 2017

Uh oh!

jiangxb1987 left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 7, 2017

Uh oh!

viirya commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Dec 7, 2017

Uh oh!

SparkQA commented Dec 7, 2017

Uh oh!

gatorsmile commented Dec 7, 2017

Uh oh!

Uh oh!

gengliangwang commented Dec 6, 2017 •

edited

Loading