-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-32932][SQL] Do not use local shuffle reader at final stage on write command #29797
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.
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
Changes from 4 commits
84134b0
b112133
f05b458
4fdee62
39dc4ca
364ee28
d391269
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,8 @@ import org.apache.spark.sql.catalyst.rules.{PlanChangeLogger, Rule} | |
| import org.apache.spark.sql.catalyst.trees.TreeNodeTag | ||
| import org.apache.spark.sql.execution._ | ||
| import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanExec._ | ||
| import org.apache.spark.sql.execution.command.DataWritingCommandExec | ||
| import org.apache.spark.sql.execution.datasources.v2.V2TableWriteExec | ||
| import org.apache.spark.sql.execution.exchange._ | ||
| import org.apache.spark.sql.execution.ui.{SparkListenerSQLAdaptiveExecutionUpdate, SparkListenerSQLAdaptiveSQLMetricUpdates, SQLPlanMetric} | ||
| import org.apache.spark.sql.internal.SQLConf | ||
|
|
@@ -102,6 +104,16 @@ case class AdaptiveSparkPlanExec( | |
| OptimizeLocalShuffleReader(conf) | ||
| ) | ||
|
|
||
| @transient private val finalStageOptimizerRules: Seq[Rule[SparkPlan]] = | ||
| context.qe.sparkPlan match { | ||
| case _: DataWritingCommandExec | _: V2TableWriteExec => | ||
| // SPARK-32932: Local shuffle reader could break partitioning that works best | ||
| // for the following writing command | ||
| queryStageOptimizerRules.filterNot(_.isInstanceOf[OptimizeLocalShuffleReader]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: indentation is one space off. |
||
| case _ => | ||
| queryStageOptimizerRules | ||
| } | ||
|
|
||
| // A list of physical optimizer rules to be applied right after a new stage is created. The input | ||
| // plan to these rules has exchange as its root node. | ||
| @transient private val postStageCreationRules = Seq( | ||
|
|
@@ -235,7 +247,7 @@ case class AdaptiveSparkPlanExec( | |
| // Run the final plan when there's no more unfinished stages. | ||
| currentPhysicalPlan = applyPhysicalRules( | ||
| result.newPlan, | ||
| queryStageOptimizerRules ++ postStageCreationRules, | ||
| finalStageOptimizerRules ++ postStageCreationRules, | ||
| Some((planChangeLogger, "AQE Final Query Stage Optimization"))) | ||
| isFinalPlan = true | ||
| executionId.foreach(onUpdatePlan(_, Seq(currentPhysicalPlan))) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,15 +26,21 @@ import org.apache.spark.scheduler.{SparkListener, SparkListenerEvent, SparkListe | |
| import org.apache.spark.sql.{Dataset, QueryTest, Row, SparkSession, Strategy} | ||
| import org.apache.spark.sql.catalyst.optimizer.{BuildLeft, BuildRight} | ||
| import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, LogicalPlan} | ||
| import org.apache.spark.sql.execution.{PartialReducerPartitionSpec, ReusedSubqueryExec, ShuffledRowRDD, SparkPlan} | ||
| import org.apache.spark.sql.connector.SimpleWritableDataSource | ||
| import org.apache.spark.sql.connector.catalog.Table | ||
| import org.apache.spark.sql.connector.expressions.{FieldReference, IdentityTransform, Transform} | ||
| import org.apache.spark.sql.execution.{PartialReducerPartitionSpec, QueryExecution, ReusedSubqueryExec, ShuffledRowRDD, SparkPlan, UnaryExecNode} | ||
| import org.apache.spark.sql.execution.command.DataWritingCommandExec | ||
| import org.apache.spark.sql.execution.datasources.v2.V2TableWriteExec | ||
| import org.apache.spark.sql.execution.exchange.{BroadcastExchangeExec, Exchange, ReusedExchangeExec, ShuffleExchangeExec} | ||
| import org.apache.spark.sql.execution.joins.{BaseJoinExec, BroadcastHashJoinExec, SortMergeJoinExec} | ||
| import org.apache.spark.sql.execution.ui.SparkListenerSQLAdaptiveExecutionUpdate | ||
| import org.apache.spark.sql.functions._ | ||
| import org.apache.spark.sql.internal.SQLConf | ||
| import org.apache.spark.sql.internal.SQLConf.PartitionOverwriteMode | ||
| import org.apache.spark.sql.test.SharedSparkSession | ||
| import org.apache.spark.sql.types.{IntegerType, StructType} | ||
| import org.apache.spark.sql.util.{CaseInsensitiveStringMap, QueryExecutionListener} | ||
| import org.apache.spark.util.Utils | ||
|
|
||
| class AdaptiveQueryExecSuite | ||
|
|
@@ -1258,4 +1264,67 @@ class AdaptiveQueryExecSuite | |
| } | ||
| } | ||
| } | ||
|
|
||
| test("SPARK-32932: Do not use local shuffle reader at final stage on write command") { | ||
| withSQLConf(SQLConf.PARTITION_OVERWRITE_MODE.key -> PartitionOverwriteMode.DYNAMIC.toString, | ||
| SQLConf.SHUFFLE_PARTITIONS.key -> "5", | ||
| SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "true") { | ||
| val data = for ( | ||
| i <- 1L to 10L; | ||
| j <- 1L to 3L | ||
| ) yield (i, j) | ||
|
|
||
| val df = data.toDF("i", "j").repartition($"j") | ||
| var noLocalReader: Boolean = false | ||
| val listener = new QueryExecutionListener { | ||
| override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit = { | ||
| qe.executedPlan match { | ||
| case plan@(_: DataWritingCommandExec | _: V2TableWriteExec) => | ||
| assert(plan.asInstanceOf[UnaryExecNode].child.isInstanceOf[AdaptiveSparkPlanExec]) | ||
| noLocalReader = collect(plan) { | ||
| case exec: CustomShuffleReaderExec if exec.isLocalReader => exec | ||
| }.isEmpty | ||
| case _ => // ignore other events | ||
| } | ||
| } | ||
| override def onFailure(funcName: String, qe: QueryExecution, | ||
| exception: Exception): Unit = {} | ||
| } | ||
| spark.listenerManager.register(listener) | ||
|
|
||
| withTable("t") { | ||
| df.write.partitionBy("j").saveAsTable("t") | ||
| sparkContext.listenerBus.waitUntilEmpty() | ||
| assert(noLocalReader) | ||
| noLocalReader = false | ||
| } | ||
|
|
||
| // Test DataSource v2 | ||
| withTempPath { f => | ||
| val path = f.getCanonicalPath | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and we can remove |
||
| val format = classOf[V2Source].getName | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. partitioned or not doesn't matter, we can test |
||
| df.write.format(format).partitionBy("j").mode("overwrite").save(path) | ||
| sparkContext.listenerBus.waitUntilEmpty() | ||
| assert(noLocalReader) | ||
| noLocalReader = false | ||
| } | ||
|
|
||
| spark.listenerManager.unregister(listener) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class V2Source extends SimpleWritableDataSource { | ||
|
|
||
| class TestingTable(options: CaseInsensitiveStringMap) | ||
| extends MyTable(options) { | ||
|
|
||
| override def partitioning(): Array[Transform] = { | ||
| Array(IdentityTransform(FieldReference("j"))) | ||
| } | ||
| } | ||
|
|
||
| override def getTable(options: CaseInsensitiveStringMap): Table = { | ||
| new TestingTable(options) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's only called once, can be a
def