peter-toth commented on code in PR #59599:
URL: https://github.com/apache/spark/pull/59599#discussion_r4339031144
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/EnsureRequirements.scala:
##########
@@ -185,7 +185,10 @@ case class EnsureRequirements(
parent.get, left, right, requiredChildDistributions)
if (newChildren.isDefined) {
children = newChildren.get
- true
+ // SPJ pushdown can update a nested join while the other side is
still a
+ // placeholder shuffle. Fall back to the shuffle-alignment path
unless both
+ // sides already report the same number of partitions.
+ children.map(_.outputPartitioning.numPartitions).distinct.length == 1
Review Comment:
**Finding 2.** When this is false, the shuffle branch below runs on
`children`, which already holds the plans `checkKeyGroupCompatible` pushed
into. The positions in `specs` were computed against the original children, so
that branch composes them a second time. Take the setup from finding 1, plus
`e4 (store INT, id INT, data STRING) PARTITIONED BY (store, id)` holding (10,
1), (20, 2) and (30, 3). Also set `requireAllClusterKeysForCoPartition=false`
and `shuffle.enabled=true`:
```sql
SELECT /*+ MERGE(e1, e2, e4) */ e1.id, e2.id, e4.id, e4.store
FROM e1 JOIN e2 ON e1.id = e2.id
JOIN e4 ON e1.id = e4.id
```
Planning fails with `ArrayIndexOutOfBoundsException: Index 1 out of bounds
for length 1` in `composeJoinKeyPositions`, AQE on and off. `e4`'s positions
`[1]` get composed with themselves. `branch-4.1` plans it with no shuffle.
(`e4.store` is selected so that the scan keeps reporting its partitioning.)
The guard also fails the existing `EnsureRequirementsSuite` test
"SPARK-41413: check compatibility when partition values mismatch". Its dummy
children keep reporting 2 and 3 partitions, so the join is now shuffled. The
`match` then falls to `fail(other.toString)`, which throws an NPE on the null
`table`. It fails locally, and CI's "sql - other tests" job is red too.
None of the new tests needs the guard. With `true` here they all pass. I'd
go back to `true`. If there's a placeholder-shuffle case that needs the guard,
please add a test for it, and start the fallback from `left` and `right` rather
than from the pushed-down children.
--
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]
