Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spark-21807][SQL]Override ++ operation in ExpressionSet to reduce clone time #19022

Closed
wants to merge 10 commits into from
Closed

[Spark-21807][SQL]Override ++ operation in ExpressionSet to reduce clone time #19022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
80af01a
The getAliasedConstraints function will take a long time when express…
eatoncys Aug 22, 2017
7444a1c
Modify name
eatoncys Aug 22, 2017
1f5e0a3
Modify name
eatoncys Aug 22, 2017
88185ff
Modify name
eatoncys Aug 22, 2017
97803ce
Override plus plus
eatoncys Aug 23, 2017
8358351
Override plus plus
eatoncys Aug 23, 2017
6ee2ffd
Add expression set test
eatoncys Aug 23, 2017
57e8a3d
Remove getAliasedConstraints test
eatoncys Aug 23, 2017
fec05d6
Change import order
eatoncys Aug 24, 2017
0762840
Change import order
eatoncys Aug 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExpressionSet.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

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

import scala.collection.mutable
import scala.collection.{mutable, GenTraversableOnce}
import scala.collection.mutable.ArrayBuffer

object ExpressionSet {
Expand Down Expand Up @@ -67,6 +67,12 @@ class ExpressionSet protected(
newSet
}

override def ++(elems: GenTraversableOnce[Expression]): ExpressionSet = {
val newSet = new ExpressionSet(baseSet.clone(), originals.clone())
elems.foreach(newSet.add)
newSet
}

override def -(elem: Expression): ExpressionSet = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

val newBaseSet = baseSet.clone().filterNot(_ == elem.canonicalized)
val newOriginals = originals.clone().filterNot(_.canonicalized == elem.canonicalized)
Expand Down
9 changes: 9 additions & 0 deletions ...atalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionSetSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,4 +210,13 @@ class ExpressionSetSuite extends SparkFunSuite {
assert((initialSet - (aLower + 1)).size == 0)

}

test("add multiple elements to set") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ++.

val initialSet = ExpressionSet(aUpper + 1 :: Nil)
val setToAddWithSameExpression = ExpressionSet(aUpper + 1 :: aUpper + 2 :: Nil)
val setToAddWithOutSameExpression = ExpressionSet(aUpper + 3 :: aUpper + 4 :: Nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: WithOut -> Without


assert((initialSet ++ setToAddWithSameExpression).size == 2)
assert((initialSet ++ setToAddWithOutSameExpression).size == 3)
}
}