Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
01e4cdf
Merge remote-tracking branch 'upstream/master'
gatorsmile Nov 13, 2015
6835704
Merge remote-tracking branch 'upstream/master'
gatorsmile Nov 14, 2015
9180687
Merge remote-tracking branch 'upstream/master'
gatorsmile Nov 14, 2015
b38a21e
SPARK-11633
gatorsmile Nov 17, 2015
d2b84af
Merge remote-tracking branch 'upstream/master' into joinMakeCopy
gatorsmile Nov 17, 2015
fda8025
Merge remote-tracking branch 'upstream/master'
gatorspark Nov 17, 2015
ac0dccd
Merge branch 'master' of https://github.com/gatorsmile/spark
gatorspark Nov 17, 2015
6e0018b
Merge remote-tracking branch 'upstream/master'
Nov 20, 2015
0546772
converge
gatorsmile Nov 20, 2015
b37a64f
converge
gatorsmile Nov 20, 2015
c2a872c
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 6, 2016
ab6dbd7
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 6, 2016
4276356
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 6, 2016
2dab708
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 7, 2016
0458770
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 8, 2016
1debdfa
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 9, 2016
763706d
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 14, 2016
4de6ec1
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 18, 2016
9422a4f
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 19, 2016
52bdf48
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 20, 2016
1e95df3
Merge remote-tracking branch 'upstream/master'
gatorsmile Jan 23, 2016
fab24cf
Merge remote-tracking branch 'upstream/master'
gatorsmile Feb 1, 2016
8b2e33b
Merge remote-tracking branch 'upstream/master'
gatorsmile Feb 5, 2016
2ee1876
Merge remote-tracking branch 'upstream/master'
gatorsmile Feb 11, 2016
b9f0090
Merge remote-tracking branch 'upstream/master'
gatorsmile Feb 12, 2016
ade6f7e
Merge remote-tracking branch 'upstream/master'
gatorsmile Feb 15, 2016
9fd63d2
Merge remote-tracking branch 'upstream/master'
gatorsmile Feb 19, 2016
5199d49
Merge remote-tracking branch 'upstream/master'
gatorsmile Feb 22, 2016
404214c
Merge remote-tracking branch 'upstream/master'
gatorsmile Feb 23, 2016
c001dd9
Merge remote-tracking branch 'upstream/master'
gatorsmile Feb 25, 2016
59daa48
Merge remote-tracking branch 'upstream/master'
gatorsmile Mar 5, 2016
41d5f64
Merge remote-tracking branch 'upstream/master'
gatorsmile Mar 7, 2016
472a6e3
Merge remote-tracking branch 'upstream/master'
gatorsmile Mar 10, 2016
0fba10a
Merge remote-tracking branch 'upstream/master'
gatorsmile Mar 12, 2016
797aabb
When doing the test, issue an exception if hitting the max iteration.
gatorsmile Mar 15, 2016
98cab44
Merge remote-tracking branch 'upstream/master' into maxIterationExcep…
gatorsmile Mar 15, 2016
f351362
address comments.
gatorsmile Mar 15, 2016
1281f36
code clean
gatorsmile Mar 15, 2016
23663fe
update comments.
gatorsmile Mar 15, 2016
cbf73b3
Merge remote-tracking branch 'upstream/master'
gatorsmile Mar 21, 2016
c08f561
Merge remote-tracking branch 'upstream/master'
gatorsmile Mar 22, 2016
474df88
Merge remote-tracking branch 'upstream/master'
gatorsmile Mar 22, 2016
23a6cfc
Merge branch 'maxIterationException' into maxIterationExceptionNew
gatorsmile Mar 22, 2016
48b2c32
address comments.
gatorsmile Mar 23, 2016
1a59fcf
clean code.
gatorsmile Mar 23, 2016
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
Prev Previous commit
Next Next commit
address comments.
  • Loading branch information
gatorsmile committed Mar 15, 2016
commit f351362475f4fd9382f36eb60150cad93aa8e864
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import scala.collection.JavaConverters._

import com.google.common.util.concurrent.AtomicLongMap

import org.apache.spark.{Logging, SparkException}
import org.apache.spark.Logging
import org.apache.spark.sql.catalyst.errors.TreeNodeException
import org.apache.spark.sql.catalyst.trees.TreeNode
import org.apache.spark.sql.catalyst.util.sideBySide
import org.apache.spark.util.Utils
Expand Down Expand Up @@ -51,18 +52,18 @@ abstract class RuleExecutor[TreeType <: TreeNode[_]] extends Logging {
*/
abstract class Strategy {
def maxIterations: Int
def throws: Boolean
def throwsExceptionUponMaxIterations: Boolean
}

/** A strategy that only runs once. */
case object Once extends Strategy {
val maxIterations = 1
val throws = false
override val maxIterations = 1
override val throwsExceptionUponMaxIterations = false
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this kind of janky to insert a field into the non-test code just to get the behavior you want in a test? how about mocking subclasses?

Copy link
Member Author

Choose a reason for hiding this comment

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

@srowen This is used in multiple components, e.g., Analyzer and Optimizer. I am not sure how to do it in a more efficient way. Could you give me some hints how we did in the Spark? Any example in the existing code base? Thanks!

}

/** A strategy that runs until fix point or maxIterations times, whichever comes first. */
case class FixedPoint(maxIterations: Int) extends Strategy {
override val throws: Boolean = if (Utils.isTesting) true else false
override val throwsExceptionUponMaxIterations: Boolean = if (Utils.isTesting) true else false
Copy link
Member

Choose a reason for hiding this comment

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

Just write = Utils.isTesting

}
/** A batch of rules. */
protected case class Batch(name: String, strategy: Strategy, rules: Rule[TreeType]*)
Expand Down Expand Up @@ -108,7 +109,11 @@ abstract class RuleExecutor[TreeType <: TreeNode[_]] extends Logging {
// Only log if this is a rule that is supposed to run more than once.
if (iteration != 2) {
val msg = s"Max iterations (${iteration - 1}) reached for batch ${batch.name}"
if (batch.strategy.throws) throw new SparkException(msg) else logTrace(msg)
if (batch.strategy.throwsExceptionUponMaxIterations) {
throw new TreeNodeException(curPlan, msg, null)
} else {
logTrace(msg)
}
}
continue = false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import org.apache.spark.sql.types.StringType
class ColumnPruningSuite extends PlanTest {

object Optimize extends RuleExecutor[LogicalPlan] {
System.setProperty("spark.testing", "true")
val batches = Batch("Column pruning", FixedPoint(100),
ColumnPruning,
CollapseProject) :: Nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import org.apache.spark.sql.catalyst.rules._
class CombiningLimitsSuite extends PlanTest {

object Optimize extends RuleExecutor[LogicalPlan] {
System.setProperty("spark.testing", "true")
val batches =
Batch("Filter Pushdown", FixedPoint(100),
ColumnPruning) ::
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import org.apache.spark.sql.catalyst.rules.RuleExecutor
import org.apache.spark.sql.catalyst.util.DateTimeUtils

class ComputeCurrentTimeSuite extends PlanTest {
System.setProperty("spark.testing", "true")
object Optimize extends RuleExecutor[LogicalPlan] {
val batches = Seq(Batch("ComputeCurrentTime", Once, ComputeCurrentTime))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import org.apache.spark.sql.types.IntegerType
class SimplifyConditionalSuite extends PlanTest with PredicateHelper {

object Optimize extends RuleExecutor[LogicalPlan] {
System.setProperty("spark.testing", "true")
val batches =
Batch("SimplifyConditionals", FixedPoint(50), SimplifyConditionals) :: Nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ import org.apache.spark.sql.catalyst.util._
* Provides helper methods for comparing plans.
*/
abstract class PlanTest extends SparkFunSuite with PredicateHelper {

System.setProperty("spark.testing", "true")
Copy link
Member

Choose a reason for hiding this comment

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

This is already set by the test framework. MIght be cleaner not to hard code it in just a few places in the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you saying SPARK_TESTING can be set by the test framework?

Copy link
Member

Choose a reason for hiding this comment

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

SPARK_TESTING is an env variable while spark.testing is a JVM system property, but yes both are set by the build for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, will make a try.


/**
* Since attribute references are given globally unique ids during analysis,
* we must normalize them to check if two different queries are identical.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@

package org.apache.spark.sql.catalyst.trees

import org.apache.spark.{SparkException, SparkFunSuite}
import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.catalyst.errors.TreeNodeException
import org.apache.spark.sql.catalyst.expressions.{Expression, IntegerLiteral, Literal}
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.catalyst.rules.{Rule, RuleExecutor}

class RuleExecutorSuite extends SparkFunSuite {
Expand Down Expand Up @@ -50,7 +52,7 @@ class RuleExecutorSuite extends SparkFunSuite {
val batches = Batch("fixedPoint", FixedPoint(10), DecrementLiterals) :: Nil
}

val message = intercept[SparkException] {
val message = intercept[TreeNodeException[LogicalPlan]] {
ToFixedPoint.execute(Literal(100))
}.getMessage
assert(message.contains("Max iterations (10) reached for batch fixedPoint"))
Expand Down