peter-toth commented on PR #58262: URL: https://github.com/apache/spark/pull/58262#issuecomment-5439993717
One thing in the verification section I would push back on, because it is the kind of claim that hardens into a constraint if it goes unanswered. > Over-coalescing is safe, under-coalescing is the bug. [...] So choosing the candidate leaving the *most* partitions, tie-breaking toward the containing set, is not merely plan-quality — dropping it would be incorrect. The premise is right and worth having written down. The conclusion does not follow, and it points the other way: if coalescing more is the safe direction, then taking the candidate that leaves the *most* partitions is the less conservative choice, not the one whose removal would break correctness. Every admitted candidate is correct on its own, whichever the ranking picks. `groupedSatisfies`' subset branch also requires `expressions.forall(_.references.size == 1)`, so each position it keeps is a function of a single cluster key. Rows that agree on *all* the cluster keys therefore agree on that one, and land in one partition. Covering a single cluster key is sufficient - which is exactly why the config is allowed to admit a subset in the first place. Measured rather than argued: in the `no GroupPartitionsExec when a join collection member needs none` test, an earlier revision of this PR picked the other candidate and inserted a node. `checkAnswer` passed - the rows were right - and only the plan was worse, 3 partitions where 4 were available. That is the whole cost of picking wrong here. So I would keep the description's "plan quality rather than correctness". If the ranking were recorded as load-bearing for correctness, the next person to look at this would not dare simplify it, and there is a simplification worth having later - the exact ranking only matters when a collection's members disagree on which positions are operation keys, which needs a join whose `outputPartitioning` unions two sides without the mixed combinations. -- 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]
