dongjoon-hyun commented on PR #58659:
URL: https://github.com/apache/spark/pull/58659#issuecomment-5713050094
Thank you for the quick turnaround, @peter-toth. I re-reviewed at
`d65de5498a3`. All four items from my previous round are addressed, and the
`childPartitioning` check plus the memo on `KeyedPartitioning` look good to me.
One remaining concern on item 1, and two nits.
### 1. Fail fast in `doExecute` when the child no longer matches
`childPartitioning`
The new `outputPartitioning` doc says the `ValidateRequirements` revert "is
also what keeps `doExecute` off the changed child". However,
`AdaptiveSparkPlanExec.optimizeQueryStage` validates only `AQEShuffleReadRule`
results. `PlanAdaptiveDynamicPruningFilters`, extension-injected
`queryStageOptimizerRules`, and the non-AQE preparation rules after
`EnsureRequirements` are not validated. If one of them hands this node a child
with a different layout, the node reports `UnknownPartitioning`, but
`doExecute` still coalesces the new child RDD with the planning-time
`grouping.partitions`, which can silently produce wrong results. The base
re-derived the grouping and re-aligned it to `expectedPartitionKeys`, so it
stayed paired with the other side in that case.
No rule is known to do this today, as you noted, so a cheap guard would be
enough. For example, in `doExecute` / `doExecuteColumnar`:
```scala
if (child.outputPartitioning != childPartitioning) {
throw SparkException.internalError(
"GroupPartitionsExec's child no longer reports the partitioning it was
planned over")
}
```
The comparison is already computed for `outputPartitioning`, so this adds no
real cost and turns a potential correctness issue into an explicit error.
### 2. Nit: the comparison cost after `PlanAdaptiveDynamicPruningFilters`
That rule rebuilds `BatchScanExec`, which recomputes `batch` /
`inputPartitions`. The equality check then compares the partition keys by value
(O(n)) instead of by reference. That is fine for correctness, but it relies on
the connector returning the same input partitions again. It may be worth
stating that assumption in the `outputPartitioning` doc.
### 3. Nit: PR description
`Generated-by: Claude Code` is missing the version the template asks for.
I'll take another look once the remaining `sql` test jobs finish.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
