[GLUTEN-11524][VL] Fix ColumnarAQEShuffleReadExec - #12691
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes ColumnarAQEShuffleReadExec canonicalization by allowing its delegate to be a generic SparkPlan (including ShuffleExchange during canonicalization), while preserving AQE shuffle reader behavior for runtime execution.
Changes:
- Refactors
ColumnarAQEShuffleReadExecto storedelegate: SparkPlanand adjustschild/output/outputPartitioning/withNewChildInternalaccordingly. - Updates stage execution mode adjustment to construct
ColumnarAQEShuffleReadExecwith the new delegate type and tweaks resize-batches handling. - Updates Velox tests to accommodate the new delegate representation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| gluten-substrait/src/main/scala/org/apache/spark/sql/execution/adaptive/ColumnarAQEShuffleReadExec.scala | Refactors delegate handling to avoid canonicalization failures and updates plan plumbing accordingly. |
| backends-velox/src/test/scala/org/apache/gluten/execution/StageExecutionModeSuite.scala | Updates assertions to derive shuffle stages via the new delegate: SparkPlan shape. |
| backends-velox/src/main/scala/org/apache/spark/sql/execution/AdjustStageExecutionMode.scala | Constructs ColumnarAQEShuffleReadExec with the new delegate type and narrows a resize-batches match. |
| backends-velox/src/main/scala/org/apache/gluten/extension/AppendBatchResizeForShuffleInputAndOutput.scala | Enables shuffle-output resize when CUDF columnar mode is on (with TODO note). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| shuffleStages.foreach { | ||
| shuffleStage => | ||
| assert(shuffleStage.shuffle.isInstanceOf[ColumnarShuffleExchangeExec]) | ||
| val exchange = shuffleStage.shuffle.asInstanceOf[ColumnarShuffleExchangeExec] | ||
| exchange => | ||
| assert( | ||
| !exchange.mapperStageMode.contains(MockGPUStageMode), |
| case r: VeloxResizeBatchesExec | ||
| if r.child.isInstanceOf[ShuffleQueryStageExec] || | ||
| r.child.isInstanceOf[AQEShuffleReadExec] => | ||
| VeloxResizeBatchesExec( | ||
| adjustExecutionMode(resizeBatches.child, stageExecutionMode), | ||
| adjustExecutionMode(r.child, stageExecutionMode), | ||
| Some(stageExecutionMode)) |
| // The child is Exchange during canonicalization. | ||
| throw new IllegalStateException( | ||
| s"Cannot get aqeReader from delegate class ${delegate.getClass.getSimpleName}.") |
| * | ||
| * @param delegate | ||
| * The AQEShuffleReadExec or ShuffleQueryStageExec. | ||
| * AQEShuffleReadExec or ShuffleQueryStageExec. Or ShuffleExchange during canonicalization. |
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
gluten-substrait/src/main/scala/org/apache/spark/sql/execution/adaptive/ColumnarAQEShuffleReadExec.scala:53
outputPartitioningnow comes fromdelegate.outputPartitioning. Whendelegateis aShuffleQueryStageExec, this can differ from the previously usedAQEShuffleReadExec(...).outputPartitioning(which is derived frompartitionSpecs). That is a semantic change and can affect downstream planning decisions that rely on AQE shuffle-reader partitioning. Consider restoring the previous behavior by derivingoutputPartitioningfromaqeReader.outputPartitioning(and similarly keep outputs consistent with the AQE reader semantics when the delegate is a stage).
override def output: Seq[Attribute] = delegate.output
override lazy val outputPartitioning: Partitioning = delegate.outputPartitioning
gluten-substrait/src/main/scala/org/apache/spark/sql/execution/adaptive/ColumnarAQEShuffleReadExec.scala:85
- The scaladoc explicitly allows
delegateto beShuffleExchangeduring canonicalization, butmetricseagerly depends onaqeReader, which will throw for any non-AQEShuffleReadExec/ShuffleQueryStageExecdelegate. This can reintroduce canonicalization-time failures if Spark touchesmetrics(e.g., for explain/UI/debug) on a canonicalized plan. To make canonicalization robust, consider makingmetrics(and any other accesses that can happen during canonicalization) safe for theShuffleExchangecase (e.g., return an empty metric map or delegate metrics when available), or tighten invariants so a non-AQE/stage delegate cannot reach paths where metrics are evaluated.
private lazy val aqeReader: AQEShuffleReadExec = {
delegate match {
case a: AQEShuffleReadExec => a
case s: ShuffleQueryStageExec =>
// Wrap ShuffleQueryStageExe with dummy PartitionSpecs by creating CoalescedPartitionSpec
// for each partition.
val partitionSpecs =
Array.tabulate(s.shuffle.numPartitions)(i => CoalescedPartitionSpec(i, i + 1))
AQEShuffleReadExec(s, partitionSpecs)
case _ =>
// The child is Exchange during canonicalization.
throw new IllegalStateException(
s"Cannot get aqeReader from delegate class ${delegate.getClass.getSimpleName}.")
}
}
@transient override lazy val metrics: Map[String, SQLMetric] = aqeReader.metrics
backends-velox/src/main/scala/org/apache/spark/sql/execution/AdjustStageExecutionMode.scala:101
- This changes behavior from adjusting
VeloxResizeBatchesExecunconditionally to only adjusting it when its child is a shuffle stage / AQE shuffle read. IfadjustExecutionModeis expected to consistently propagatestageExecutionModethrough the plan, this introduces a special case where the resize node’s stage mode may remain stale while its subtree is updated via the default case. Consider either (a) keeping the unconditional handling, or (b) adding an explicit else branch that still updates theVeloxResizeBatchesExec’s stage mode while recursing, to avoid mixed or inconsistent stage-mode annotations.
case r: VeloxResizeBatchesExec
if r.child.isInstanceOf[ShuffleQueryStageExec] ||
r.child.isInstanceOf[AQEShuffleReadExec] =>
VeloxResizeBatchesExec(
adjustExecutionMode(r.child, stageExecutionMode),
Some(stageExecutionMode))
|
Run Gluten Clickhouse CI on x86 |
ColumnarAQEShuffleReadExecis a wrapper forShuffleQueryStageExecor a replacement forAQEShuffleReadExec. Its execution mode determines whether the shuffle reader outputs VeloxRowVectorsfor the CPU pipeline orCudfVectorsfor the GPU pipeline.However, during canonicalization, the
delegatefield can be set toShuffleExchange, which currently causes a failure. This PR fixes the issue.Related issue: #11524