dongjoon-hyun commented on PR #58942: URL: https://github.com/apache/spark/pull/58942#issuecomment-5763610046
Two things to add on top of @peter-toth's review rather than repeat it. **Finding 1 reproduces, with numbers.** On `bb183663ba8`, the projection half of his finding is not hypothetical. Taking your own `ValidateRequirementsSuite` case "the same keys in a different order are not aligned" and flipping only the config: ``` [1,1,2] vs [1,2,1], subsetConf=false -> validate=false [1,1,2] vs [1,2,1], subsetConf=true -> validate=true [1,1,2] (3 partitions) vs [1,2] (2 partitions), subsetConf=true -> validate=true ``` At the catalyst level `specsForPairing` reports the divergence directly: ``` left child numPartitions = 3 left spec numPartitions = 2 right child numPartitions = 3 right spec numPartitions = 2 compatible = true ``` So the config default is the only reason that test still passes, and the second line means the validator stops noticing that two children have different partition counts at all. **`coPartitioning` is not only the join path.** It is true for any operator with more than one child all requiring `ClusteredDistribution`, which includes `CoGroupExec` (`objects.scala:637`) and `FlatMapCoGroupsInBatchExec` (`FlatMapCoGroupsInBatchExec.scala:56-59`). For a cogroup, "the two sides agree key by key while neither is grouped" is not a sufficient input: it pairs left group with right group per partition, so a key split across two partitions on each side yields two partial cogroups rather than merely a missing shuffle. The comment in `validateInternal` argues every non-join co-partitioning operator's children are grouped before the rule is done, and that holds today - `checkKeyGroupCompatible` returns `None` for a non-join parent (`EnsureRequirements.scala:598-600`), so `resolveEachChild` groups them. But `ValidateRequirements` is also the gate for third-party `AQEShuffleReadRule`s registered through `adaptiveRulesHolder` (`AdaptiveSparkPlanExec.optimizeQueryStage`), which is exactly where an invariant `EnsureRequirements` maintains stops being self-evident. Whatever shape the narrowing in finding 1 takes, scoping the per-side exemption to the operators the pairing argument actually covers would keep the guard for the rest. -- 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]
