dongjoon-hyun commented on PR #58942:
URL: https://github.com/apache/spark/pull/58942#issuecomment-5763541408
Thanks for working on this — the AQE coalescing problem is real and the
pairing
approach makes sense. One issue before this lands.
`specsForPairing` builds the spec with `k.createShuffleSpec(distribution)`.
Under
`spark.sql.sources.v2.bucketing.allowJoinKeysSubsetOfPartitionKeys=true`
that method returns `project(joinKeyPositions).toGrouped`, i.e. a **deduped
and
re-sorted** layout the child does not actually hold. Since the per-side
`satisfies` check is waived for exactly these children, nothing else catches
the
difference.
Reproduced on this branch (`bb183663ba8`). In `ValidateRequirementsSuite`,
this
is your own "the same keys in a different order are not aligned" case with
the
config flipped:
```scala
withSQLConf(SQLConf.V2_BUCKETING_ALLOW_KEYS_SUBSET_OF_PARTITION_KEYS.key ->
"true") {
val left = DummySparkPlan(outputPartitioning =
KeyedPartitioning(Seq(a), Seq(InternalRow(1), InternalRow(1),
InternalRow(2))))
val right = DummySparkPlan(outputPartitioning =
KeyedPartitioning(Seq(b), Seq(InternalRow(1), InternalRow(2),
InternalRow(1))))
ValidateRequirements.validate(
ShuffledHashJoinExec(Seq(a), Seq(b), Inner, BuildLeft, None, left,
right))
}
```
```
misaligned pair: subsetConf=false -> validate=false
misaligned pair: subsetConf=true -> validate=true
3-partition child vs 2-partition sibling (subset conf on) -> validate=true
```
At the catalyst level the divergence is visible directly:
```
left child numPartitions = 3 left spec numPartitions = 2
right child numPartitions = 3 right spec numPartitions = 2
compatible = true
```
So two consequences:
1. Two ungrouped sides whose key sequences do **not** line up position by
position (`[1,1,2]` vs `[1,2,1]`) both collapse to `[1,2]` and pair. Your
test rejects this shape today only because it runs at the config's `false`
default.
2. An ungrouped 3-partition side pairs with a grouped 2-partition sibling, so
the validator no longer notices that the two children have different
partition counts.
Note the difference from `EnsureRequirements.createKeyedShuffleSpecs`: the
planner applies `toGrouped` because it is about to insert the
`GroupPartitionsExec` that makes it true. A validator reading a finished plan
has no such node to point at, so it should judge the pair on the layouts the
children actually report. Building the pairing specs without `project`/
`toGrouped` would fix both cases and, as far as I can tell, still admits the
partially-clustered pair this PR is after — both sides report their raw keys
and
`describesSameKeys` compares them index by index.
Two smaller points:
- `coPartitioning` is true for any operator with >1 children all requiring
`ClusteredDistribution`, which includes `CoGroupExec` (`objects.scala:637`)
and `FlatMapCoGroupsInBatchExec`, not just joins. For a cogroup, "aligned
but
neither side grouped" is not a sufficient input — it would emit partial
cogroups rather than merely miss a shuffle. The comment argues
`EnsureRequirements` groups them (it does — `checkKeyGroupCompatible`
returns
`None` for non-join parents), but `ValidateRequirements` is also the gate
for
third-party `AQEShuffleReadRule`s registered via `adaptiveRulesHolder`.
Restricting the exemption to the join path named in the comment would keep
the fix and the guard.
- `PartitioningCollection.createShuffleSpec` and `maySatisfyAfterProjection`
still justify their admission set with "ValidateRequirements builds a spec
from a finished plan through here" / "the caller feeds
`ValidateRequirements`
as well as the planner". Neither is on that path anymore, so those should
be
updated to name the planner.
--
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]
