Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,16 @@ object SQLConf {
.booleanConf
.createWithDefault(true)

val REDUCE_POST_SHUFFLE_PARTITIONS_FOR_REPARTITION =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I probably won't add this config. If users call Dataset#repartition, we have to respect it and not change it.

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.

I initially didn't add this config. Later I found with that change, the test("Union two datasets with different pre-shuffle partition number") won't test what we want to test, because it was written using repartition. Probably I can rewrite that test without using repartition.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can remove that test. Excluding repartition, the pre-shuffle num partitions are always the same (200 by default).

We do need to have a test for repartition though.

buildConf("spark.sql.adaptive.reducePostShufflePartitionsForRepartition")
.doc("When true and reducePostShufflePartitions in adaptive execution is enabled, it also " +
"reduces the number of post-shuffle partitions for shuffles introduced by repartition. " +
"By default this is false, so users get the desired partition number by calling " +
"repartition even in adaptive execution.")
.internal()
.booleanConf
.createWithDefault(false)

val SHUFFLE_MIN_NUM_POSTSHUFFLE_PARTITIONS =
buildConf("spark.sql.adaptive.minNumPostShufflePartitions")
.doc("The advisory minimum number of post-shuffle partitions used in adaptive execution.")
Expand Down Expand Up @@ -1951,6 +1961,9 @@ class SQLConf extends Serializable with Logging {

def reducePostShufflePartitionsEnabled: Boolean = getConf(REDUCE_POST_SHUFFLE_PARTITIONS_ENABLED)

def reducePostShufflePartitionsForRepartition: Boolean =
getConf(REDUCE_POST_SHUFFLE_PARTITIONS_FOR_REPARTITION)

def minNumPostShufflePartitions: Int =
getConf(SHUFFLE_MIN_NUM_POSTSHUFFLE_PARTITIONS)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,9 @@ abstract class SparkStrategies extends QueryPlanner[SparkPlan] {

case logical.Repartition(numPartitions, shuffle, child) =>
if (shuffle) {
ShuffleExchangeExec(RoundRobinPartitioning(numPartitions), planLater(child)) :: Nil
val supportAdaptive = conf.reducePostShufflePartitionsForRepartition
ShuffleExchangeExec(RoundRobinPartitioning(numPartitions),
planLater(child), supportAdaptive) :: Nil
} else {
execution.CoalesceExec(numPartitions, planLater(child)) :: Nil
}
Expand Down Expand Up @@ -736,7 +738,8 @@ abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
case r: logical.Range =>
execution.RangeExec(r) :: Nil
case r: logical.RepartitionByExpression =>
exchange.ShuffleExchangeExec(r.partitioning, planLater(r.child)) :: Nil
val supportAdaptive = conf.reducePostShufflePartitionsForRepartition
exchange.ShuffleExchangeExec(r.partitioning, planLater(r.child), supportAdaptive) :: Nil
case ExternalRDD(outputObjAttr, rdd) => ExternalRDDScanExec(outputObjAttr, rdd) :: Nil
case r: LogicalRDD =>
RDDScanExec(r.output, r.rdd, "ExistingRDD", r.outputPartitioning, r.outputOrdering) :: Nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ case class ShuffleQueryStageExec(
case _ =>
}
}

def supportAdaptive: Boolean = plan.supportAdaptive

@gcz2022 gcz2022 Jul 12, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

:nit -> exchangeSupportAdaptive? Or canChangeNumPartition
QueryStage is already a part of adaptive execution

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,16 @@ case class ReduceNumShufflePartitions(conf: SQLConf) extends Rule[SparkPlan] {
// If not all leaf nodes are query stages, it's not safe to reduce the number of
// shuffle partitions, because we may break the assumption that all children of a spark plan
// have same number of output partitions.
return plan
}

val shuffleStages = plan.collect {
case stage: ShuffleQueryStageExec => stage
case ReusedQueryStageExec(_, stage: ShuffleQueryStageExec, _) => stage
}
if (!shuffleStages.forall(_.supportAdaptive)) {
plan
} else {
val shuffleStages = plan.collect {
case stage: ShuffleQueryStageExec => stage
case ReusedQueryStageExec(_, stage: ShuffleQueryStageExec, _) => stage
}
val shuffleMetrics = shuffleStages.map { stage =>
val metricsFuture = stage.mapOutputStatisticsFuture
assert(metricsFuture.isCompleted, "ShuffleQueryStageExec should already be ready")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ case class EnsureRequirements(conf: SQLConf) extends Rule[SparkPlan] {
val defaultPartitioning = distribution.createPartitioning(targetNumPartitions)
child match {
// If child is an exchange, we replace it with a new one having defaultPartitioning.
case ShuffleExchangeExec(_, c) => ShuffleExchangeExec(defaultPartitioning, c)
case ShuffleExchangeExec(_, c, _) => ShuffleExchangeExec(defaultPartitioning, c)
case _ => ShuffleExchangeExec(defaultPartitioning, child)
}
}
Expand Down Expand Up @@ -191,7 +191,7 @@ case class EnsureRequirements(conf: SQLConf) extends Rule[SparkPlan] {

def apply(plan: SparkPlan): SparkPlan = plan.transformUp {
// TODO: remove this after we create a physical operator for `RepartitionByExpression`.
case operator @ ShuffleExchangeExec(upper: HashPartitioning, child) =>
case operator @ ShuffleExchangeExec(upper: HashPartitioning, child, _) =>
child.outputPartitioning match {
case lower: HashPartitioning if upper.semanticEquals(lower) => child
case _ => operator
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ import org.apache.spark.util.collection.unsafe.sort.{PrefixComparators, RecordCo
*/
case class ShuffleExchangeExec(
override val outputPartitioning: Partitioning,
child: SparkPlan) extends Exchange {
child: SparkPlan,
supportAdaptive: Boolean = true) extends Exchange {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we remove the config, I'd probably call this canChangeNumPartition

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

+1


// NOTE: coordinator can be null after serialization/deserialization,
// e.g. it can be null on the Executor side
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1365,7 +1365,7 @@ class DatasetSuite extends QueryTest with SharedSQLContext {
val agg = cp.groupBy('id % 2).agg(count('id))

agg.queryExecution.executedPlan.collectFirst {
case ShuffleExchangeExec(_, _: RDDScanExec) =>
case ShuffleExchangeExec(_, _: RDDScanExec, _) =>
case BroadcastExchangeExec(_, _: RDDScanExec) =>
}.foreach { _ =>
fail(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,7 @@ class ReduceNumShufflePartitionsSuite extends SparkFunSuite with BeforeAndAfterA

test("Union two datasets with different pre-shuffle partition number") {
val test: SparkSession => Unit = { spark: SparkSession =>
spark.conf.set(SQLConf.REDUCE_POST_SHUFFLE_PARTITIONS_FOR_REPARTITION.key, "true")
val dataset1 = spark.range(3)
val dataset2 = spark.range(3)

Expand All @@ -592,4 +593,40 @@ class ReduceNumShufflePartitionsSuite extends SparkFunSuite with BeforeAndAfterA
}
withSparkSession(test, 100, None)
}

test("Do not reduce the number of shuffle partition for repartition") {
val test: SparkSession => Unit = { spark: SparkSession =>
spark.conf.set(SQLConf.REDUCE_POST_SHUFFLE_PARTITIONS_FOR_REPARTITION.key, "false")
val ds = spark.range(3)
val resultDf = ds.repartition(2, ds.col("id")).toDF()

checkAnswer(resultDf,
Seq((0), (1), (2)).map(i => Row(i)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is just Seq(0, 1, 2), right?

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.

True. Good catch.

val finalPlan = resultDf.queryExecution.executedPlan
.asInstanceOf[AdaptiveSparkPlanExec].executedPlan
assert(finalPlan.collect { case p: CoalescedShuffleReaderExec => p }.length == 0)
}
withSparkSession(test, 200, None)
}

test("Reduce the number of shuffle partition for repartition") {
val test: SparkSession => Unit = { spark: SparkSession =>
spark.conf.set(SQLConf.REDUCE_POST_SHUFFLE_PARTITIONS_FOR_REPARTITION.key, "true")
val ds = spark.range(3)
val resultDf = ds.repartition(2, ds.col("id")).toDF()

checkAnswer(resultDf,
Seq((0), (1), (2)).map(i => Row(i)))
val finalPlan = resultDf.queryExecution.executedPlan
.asInstanceOf[AdaptiveSparkPlanExec].executedPlan
val shuffleReaders = finalPlan.collect {
case reader: CoalescedShuffleReaderExec => reader
}
assert(shuffleReaders.length === 1)
shuffleReaders.foreach { reader =>
assert(reader.outputPartitioning.numPartitions === 1)
}
}
withSparkSession(test, 200, None)
}
}