Skip to content

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

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

Closed
eatoncys 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 8 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.{GenTraversableOnce, mutable}

@dongjoon-hyun dongjoon-hyun Aug 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can do style check by running dev/scalastyle.

@viirya viirya Aug 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@eatoncys This breaks scala style check.

@eatoncys eatoncys Aug 24, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, modified it, thanks.

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 = {

@viirya viirya Aug 23, 2017

Copy link
Copy Markdown
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") {

@dongjoon-hyun dongjoon-hyun Aug 23, 2017

Copy link
Copy Markdown
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?

@viirya viirya Aug 23, 2017

Copy link
Copy Markdown
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 setToAdd = ExpressionSet(aUpper + 2 :: aUpper + 3 :: Nil)

@gatorsmile gatorsmile Aug 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)?

@eatoncys eatoncys Aug 24, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have modified the name to setToAddWithOutSameExpression.


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