[Spark-21807][SQL]Override ++ operation in ExpressionSet to reduce clone time by eatoncys · Pull Request #19022 · apache/spark

eatoncys · 2017-08-22T08:57:47Z

What changes were proposed in this pull request?

The getAliasedConstraints fuction in LogicalPlan.scala will clone the expression set when an element added,
and it will take a long time. This PR add a function to add multiple elements at once to reduce the clone time.

Before modified, the cost of getAliasedConstraints is:
100 expressions: 41 seconds
150 expressions: 466 seconds

After modified, the cost of getAliasedConstraints is:
100 expressions: 1.8 seconds
150 expressions: 6.5 seconds

The test is like this:
test("getAliasedConstraints") {
val expressionNum = 150
val aggExpression = (1 to expressionNum).map(i => Alias(Count(Literal(1)), s"cnt$i")())
val aggPlan = Aggregate(Nil, aggExpression, LocalRelation())

val beginTime = System.currentTimeMillis()
val expressions = aggPlan.validConstraints
println(s"validConstraints cost: ${System.currentTimeMillis() - beginTime}ms")
// The size of Aliased expression is n * (n - 1) / 2 + n
assert( expressions.size === expressionNum * (expressionNum - 1) / 2 + expressionNum)

}

(Please fill in changes proposed in this fix)

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Run new added test.

Please review http://spark.apache.org/contributing.html before opening a pull request.

…ion is greater than 100

viirya · 2017-08-23T08:04:35Z

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

    }
  }

+  def addMultiExpressions(elems: Set[Expression]): ExpressionSet = {


GenTraversableOnce instead of Set?

viirya · 2017-08-23T08:11:54Z

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

    }
  }

+  def addMultiExpressions(elems: Set[Expression]): ExpressionSet = {


Override ++ instead of addMultiExpressions? As it is a Set, we better follow Set's semantics.

Ok, Modified it, thanks.

viirya · 2017-08-23T08:16:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala

      case a @ Alias(e, _) =>
        // For every alias in `projectList`, replace the reference in constraints by its attribute.
-        allConstraints ++= allConstraints.map(_ transform {
+        val replacedElement = allConstraints.map(_ transform {


By overriding ++, we don't need to change those.

viirya · 2017-08-23T08:17:29Z

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

    }
  }

+  def addMultiExpressions(elems: Set[Expression]): ExpressionSet = {


Let's add a related test in ExpressionSetSuite.

Ok, Added, thanks.

viirya · 2017-08-23T08:20:58Z

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

+
+    val expressions = aggPlan.validConstraints
+    // The size of Aliased expression is n * (n - 1) / 2 + n
+    assert( expressions.size === expressionNum * (expressionNum - 1) / 2 + expressionNum)


Relying on the size of validConstraints may be fragile. I think we only need to test if the new API of ExpressionSet works. There should be other constraints test to make sure we produce correct constraints.

dongjoon-hyun · 2017-08-23T11:26:02Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionSetSuite.scala


  }
+
+  test("add multiple elements to set") {


This test case doesn't fail without the above code, does it?

Yap, previous ++ works without problem. The override version goes to reduce the time cost. This test is just used to make sure we don't mess up behavior of ++.

dongjoon-hyun · 2017-08-23T11:29:43Z

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

 package org.apache.spark.sql.catalyst.expressions

-import scala.collection.mutable
+import scala.collection.{GenTraversableOnce, mutable}


You can do style check by running dev/scalastyle.

@eatoncys This breaks scala style check.

Ok, modified it, thanks.

viirya · 2017-08-23T14:05:33Z

We can edit the PR title and description to show the changes clearly.

viirya · 2017-08-23T14:07:41Z

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

+    newSet
+  }
+
  override def -(elem: Expression): ExpressionSet = {


I guess we don't use -- frequently or in hot code path. We may add it when we need it.

viirya · 2017-08-23T14:08:44Z

cc @gatorsmile @HyukjinKwon May you help to trigger Jenkins tests? Thanks.

HyukjinKwon · 2017-08-23T14:10:54Z

ok to test

SparkQA · 2017-08-23T14:13:56Z

Test build #81035 has finished for PR 19022 at commit 57e8a3d.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-08-23T16:30:26Z

add to whitelist

SparkQA · 2017-08-23T16:34:10Z

Test build #81039 has finished for PR 19022 at commit 57e8a3d.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-08-23T16:48:40Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionSetSuite.scala

+  test("add multiple elements to set") {
+    val initialSet = ExpressionSet(aUpper + 1 :: Nil)
+    val setToAddWithSameExpression = ExpressionSet(aUpper + 1 :: aUpper + 2 :: Nil)
+    val setToAdd = ExpressionSet(aUpper + 2 :: aUpper + 3 :: Nil)


Not very sure what you want to test in the second one. Are you want to test the behavior of adding a set of expressions that do not exist in the initial set?
-> ExpressionSet(aUpper + 3 :: aUpper + 4 :: Nil)?

Yes, I have modified the name to setToAddWithOutSameExpression.

SparkQA · 2017-08-24T03:22:59Z

Test build #81057 has finished for PR 19022 at commit 0762840.

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

gatorsmile · 2017-08-24T03:34:10Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionSetSuite.scala

+  test("add multiple elements to set") {
+    val initialSet = ExpressionSet(aUpper + 1 :: Nil)
+    val setToAddWithSameExpression = ExpressionSet(aUpper + 1 :: aUpper + 2 :: Nil)
+    val setToAddWithOutSameExpression = ExpressionSet(aUpper + 3 :: aUpper + 4 :: Nil)


Nit: WithOut -> Without

gatorsmile · 2017-08-24T03:34:23Z

LGTM

gatorsmile · 2017-08-24T03:36:18Z

Thanks! Merging to master.

You can fix this in your future PRs.

eatoncys added 4 commits August 22, 2017 15:47

The getAliasedConstraints function will take a long time when express…

80af01a

…ion is greater than 100

Modify name

7444a1c

Modify name

1f5e0a3

Modify name

88185ff

viirya reviewed Aug 23, 2017

View reviewed changes

eatoncys added 4 commits August 23, 2017 16:58

Override plus plus

97803ce

Override plus plus

8358351

Add expression set test

6ee2ffd

Remove getAliasedConstraints test

57e8a3d

dongjoon-hyun reviewed Aug 23, 2017

View reviewed changes

viirya reviewed Aug 23, 2017

View reviewed changes

gatorsmile reviewed Aug 23, 2017

View reviewed changes

eatoncys added 2 commits August 24, 2017 08:44

Change import order

fec05d6

Change import order

0762840

eatoncys changed the title ~~[Spark-21807][SQL]The getAliasedConstraints function in LogicalPlan will take a long time when number of expressions is greater than 100~~ [Spark-21807][SQL]Override ++ operation in ExpressionSet to reduce clone time Aug 24, 2017

gatorsmile reviewed Aug 24, 2017

View reviewed changes

asfgit closed this in b8aaef4 Aug 24, 2017

jstokes added a commit to amperity/spark that referenced this pull request Jul 12, 2018

Apply patch from apache#19022

327af50

zheniantoushipashi mentioned this pull request Jul 5, 2019

#39 performance issue in fuction getAliasedConstraints of LogicalPlan Kyligence/spark#27

Closed

Conversation

eatoncys commented Aug 22, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

viirya Aug 23, 2017 • 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!

viirya Aug 23, 2017 • 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!

viirya commented Aug 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Aug 23, 2017

Uh oh!

HyukjinKwon commented Aug 23, 2017

Uh oh!

SparkQA commented Aug 23, 2017

Uh oh!

gatorsmile commented Aug 23, 2017

Uh oh!

SparkQA commented Aug 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Aug 24, 2017

Uh oh!

gatorsmile commented Aug 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

viirya Aug 23, 2017 •

edited

Loading

viirya Aug 23, 2017 •

edited

Loading