peter-toth commented on PR #58659: URL: https://github.com/apache/spark/pull/58659#issuecomment-5718191565
Thanks @cloud-fan. All five are addressed in e0cc1ebf53a. **The columnar guard.** You are right, and the test was weaker than it looked: removing the `doExecuteColumnar` call left it green. The invariant test now drives a columnar child through `executeColumnar()` as well, and it fails with either call removed. **The factory doc.** Also right, and the claim was false. `plannedPartitioning` holds `AttributeReference`s and is normalized. What actually makes `doCanonicalize`'s `copy` safe is that `super.doCanonicalize()` has already normalized both partitioning fields by then, and the reducer rewrite touches neither. Reworded to say that. **`keysSatisfy` and the link.** Both taken. The predicate says nothing about duplicate keys, which a grouping node also coalesces, so an admitted layout can still need one and `satisfies` is what adds `isGrouped`. And `[[GroupPartitionsExec]]` does not resolve from `sql/catalyst`; both occurrences are plain backticks now, like the other twenty-five in that file. **The `pickCoPartitionTarget` doc.** Taken. It still said the largest partition count picks the target, where a child with no `ShuffleExchangeLike` comes first. Worth noting that the sentence and the preference are both older than this PR, so the contradiction was already there; this change moved the sentence into a new scaladoc, so it is fixed here. **The eight malformed sentences**, with one correction. Six of them are not new: `in the child's on top`, `groups partition every input index`, `it's physical size less than`, `the number represent`, `either one of below` and `according to other shuffle spec` all appear on master and show as added lines only because the restructure moves those blocks into new methods. The two written here are the dangling `raw partition keys'` and `The claim goes on to declare lives`. I fixed all eight rather than only mine, since leaving the older six would have left the new methods reading badly. For the rename: thank you, I will leave it in then. -- 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]
