peter-toth commented on PR #58814: URL: https://github.com/apache/spark/pull/58814#issuecomment-5694859737
@ulysses-you I have to take back [the unblock](https://github.com/apache/spark/pull/58814#issuecomment-5688229206). Reviewing #58681 on a rebase found a case this PR makes worse, and it is the case #58681 turns on by default. **What I measured.** I merged #58681's head `8c5c30e` with `apache/master` at `24e232ceed5`, clean, and ran `KeyGroupedPartitioningSuite`, `EnsureRequirementsSuite` and `GroupPartitionsExecSuite`, 278 tests. One fails: ``` - SPARK-59050: SPJ: unknown-keyed partitioning still joins a subset-keyed partner *** FAILED *** 2 did not equal 1 expected 1 shuffles, got 2 ``` `KeyGroupedPartitioningSuite.scala:7655`, a test neither PR touches. On the same tree, reverting only `spark.sql.sources.v2.bucketing.partition.filter.enabled` to `false` makes it pass with one shuffle. **Why.** `r` is marked and declares `[1, 2]`, `u` declares `[1]`. With filtering on, `mergeAndDedupPartitions` intersects to `[1]`. `r` then regroups 2 groups into 1, `identityGrouping` fails on the count clause, the claim goes, and the gate this PR added declines the pairing. Before this PR that plan kept its storage-partitioned join and failed `ValidateRequirements`. Now it shuffles. **Why this differs from the SPARK-59050 regrouping test.** There the merged list is sorted and the marked side's declared order is not the sorted order. No key list would have held that pairing together, so the fourth shuffle bought a valid plan and live AQE, and that trade was right. Here the merged list did not have to be the intersection. The union is `[1, 2]`, which is the marked side's declared list exactly, so its regrouping is the identity, the claim survives and the join keeps its single shuffle. As it stands we pay a shuffle of both sides to skip reading one key group. **The fix.** Where exactly one side is marked, build the merged key list as that side's `partitionKeys` in its declared order. `areKeysCompatible`'s marked path (`partitioning.scala:1778`) already requires the other side's keys to be a subset of those, so that list covers both sides and the marked side's regrouping is the identity by construction. Where both sides are marked the same path requires equal key sequences, so the rule yields the same list. It also covers the neighbouring shape, where the merged set equals the marked side's declared set but `mergeAndDedupPartitions` re-sorts it. Partial clustering is the part that needs care, since it rewrites the split counts on top of that list. It forfeits partition filtering's pruning on a marked pair. That is the right price: one key group unread against a shuffle of both sides. **Could you hold the #58681 rebase until the follow-up is up?** Sorry for the churn. I would rather fix it here than have you rebase twice, and it should be a short one: the rule is local to the single merge call site, and the gate this PR added is what makes the attempt safe. I am putting the follow-up up now and will ping you on #58681 when it is ready to rebase onto. -- 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]
