Skip to content

Commit 3e95783

Browse files
committed
[SPARK-59289][SQL] Refuse to execute a grouping over a child it was not planned over
`GroupPartitionsExec.doExecute` and `doExecuteColumnar` compare `child.outputPartitioning` against `childPartitioning` and throw an internal error when they differ, rather than coalescing the new child's RDD on `grouping.partitions`, which indexes the child's partitions as of planning. Giving up the keyed claim, which this PR already does, only gets `ValidateRequirements` to reject such a plan. That is enough for the case it was written for, an `AQEShuffleReadExec` landing over a keyed shuffle stage, because `AdaptiveSparkPlanExec.optimizeQueryStage` reverts a rule whose result fails validation. It validates an `AQEShuffleReadRule`'s result and nothing else, so `PlanAdaptiveDynamicPruningFilters`, an extension-injected query stage rule, or a preparation rule after `EnsureRequirements` would not be caught. The base survived those by re-deriving the grouping against the new child. No rule is known to hand this node a child reporting a different partitioning, so the comparison is free in practice and the throw is unreachable. It turns a silent wrong result into an explicit error if that changes. The invariant test now asserts it for each of the three ways a child can report something else. Raised by dongjoon-hyun, who also pointed out that the comparison is by reference only while the child hands back the same object: `PlanAdaptiveDynamicPruningFilters` rebuilds `BatchScanExec`, so there it compares the partition keys by value and reads equal only while the connector returns the same input partitions. That assumption is now in `outputPartitioning`'s doc, along with the correction that the AQE revert does not cover every rule.
1 parent d65de54 commit 3e95783

2 files changed

Lines changed: 32 additions & 4 deletions

File tree

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/GroupPartitionsExec.scala

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,15 +102,19 @@ case class GroupPartitionsExec(
102102
* node a child reporting something else invalidates both, so the keyed claim is dropped and only
103103
* the physical output count is reported. `ValidateRequirements` then finds the parent's
104104
* distribution unsatisfied and `AdaptiveSparkPlanExec` reverts the rewrite, which is what an
105-
* `AQEShuffleReadExec` landing over a keyed shuffle stage relies on. That revert is also what
106-
* keeps `doExecute` off the changed child, since it still coalesces on the planning-time
107-
* `grouping.partitions`. The base relied on the same revert, having re-derived to get there.
105+
* `AQEShuffleReadExec` landing over a keyed shuffle stage relies on. Only an `AQEShuffleReadRule`
106+
* is validated that way, so `checkChildStillMatches` covers the rest at execution.
108107
*
109108
* Re-deriving instead would apply a recipe chosen for one pairing to a different child, yielding
110109
* a claim nothing validated against the other side. It would also buy nothing today: no rule is
111110
* known to report a *different* `KeyedPartitioning`, so the reachable outcomes are an equal one,
112111
* where this check passes, and `UnknownPartitioning`, where re-deriving reports the same thing.
113112
*
113+
* The comparison is by reference for a child that reports the same object, which the columnar and
114+
* codegen wrappers do. `PlanAdaptiveDynamicPruningFilters` rebuilds `BatchScanExec`, so there it
115+
* compares the partition keys by value, and reads equal only while the connector returns the same
116+
* input partitions for the same scan.
117+
*
114118
* Asked on the read rather than in `withNewChildInternal` because canonicalization rebuilds this
115119
* node over a canonicalized child whose partitioning cannot be read at all: a canonicalized
116120
* `BatchScanExec` throws from `reportedKeyedPartitioning`.
@@ -308,7 +312,24 @@ case class GroupPartitionsExec(
308312
sparkContext, executionId, driverAccumUpdates.toSeq)
309313
}
310314

315+
/**
316+
* `grouping.partitions` indexes the child's partitions as of planning, so a child that no longer
317+
* reports what this node was decided over would be coalesced on the wrong indices.
318+
* `outputPartitioning` gives up its keyed claim in that case, which lets `ValidateRequirements`
319+
* reject the plan, but only an `AQEShuffleReadRule`'s result is validated (see
320+
* `AdaptiveSparkPlanExec.optimizeQueryStage`), so nothing catches a rewrite from any other rule.
321+
* No rule is known to perform one; this makes the failure explicit rather than silent if one
322+
* starts.
323+
*/
324+
private def checkChildStillMatches(): Unit = {
325+
if (child.outputPartitioning != childPartitioning) {
326+
throw SparkException.internalError(
327+
"GroupPartitionsExec's child no longer reports the partitioning it was planned over")
328+
}
329+
}
330+
311331
override protected def doExecute(): RDD[InternalRow] = {
332+
checkChildStillMatches()
312333
sendDriverMetrics()
313334
if (groupedPartitions.isEmpty) {
314335
sparkContext.emptyRDD
@@ -329,6 +350,7 @@ case class GroupPartitionsExec(
329350
override def supportsColumnar: Boolean = child.supportsColumnar && !usesSortedMerge
330351

331352
override protected def doExecuteColumnar(): RDD[ColumnarBatch] = {
353+
checkChildStillMatches()
332354
sendDriverMetrics()
333355
if (groupedPartitions.isEmpty) {
334356
sparkContext.emptyRDD

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/GroupPartitionsExecSuite.scala

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717

1818
package org.apache.spark.sql.execution.datasources.v2
1919

20-
import org.apache.spark.SparkContext
20+
import org.apache.spark.{SparkContext, SparkException}
2121
import org.apache.spark.rdd.RDD
2222
import org.apache.spark.sql.catalyst.InternalRow
2323
import org.apache.spark.sql.catalyst.expressions.{Ascending, Attribute, AttributeReference, SortOrder, TransformExpression}
@@ -668,6 +668,12 @@ class GroupPartitionsExecSuite extends SharedSparkSession {
668668
s"a child reporting $changed invalidates the grouping, so the claim goes")
669669
assert(rebuilt.plannedPartitioning === gpe.outputPartitioning,
670670
"what it was planned to report is still carried, it is just no longer reported")
671+
672+
// Giving up the claim is only half of it. `ValidateRequirements` rejects such a plan, but
673+
// `AdaptiveSparkPlanExec.optimizeQueryStage` validates an `AQEShuffleReadRule`'s result and
674+
// nothing else, so execution refuses rather than coalescing the new child on the old indices.
675+
val e = intercept[SparkException](rebuilt.execute())
676+
assert(e.getMessage.contains("no longer reports the partitioning it was planned over"))
671677
}
672678
}
673679
}

0 commit comments

Comments
 (0)