ykisana commented on PR #59199: URL: https://github.com/apache/spark/pull/59199#issuecomment-6041756054
> Re-checked through [`0373a90`](https://github.com/apache/spark/pull/59199/commits/0373a904aa56b49751046a54189366eae0f53de2) -- finding 1 resolved ([SPARK-60011](https://issues.apache.org/jira/browse/SPARK-60011) tracks the pruning gap, and the description says so). The code has not changed since round 1, so the findings below were there then too. > ### Non-blocking > > * **2.** _The scan-merge effect is untested (late catch):_ the third "why" bullet says `PlanMerger` declined to merge two derivations of a nested ordering, but only the `SortExec` path has a test. A `MergeSubplansSuite` case would pin it. I measured that it fails on the merge base and passes here. [inline](https://github.com/apache/spark/pull/59199#discussion_r4207877792) > > > ### Minor > > * **3.** _The user-facing section is broader than the fix (late catch):_ "no longer contains a redundant `SortExec`" holds only while the scan reads the whole struct, and the scan-merge change is not mentioned. [inline](https://github.com/apache/spark/pull/59199#discussion_r4207877812) > > * **4.** _Description nits (late catch):_ the "why" list is numbered 1, 3, 4. The AI section lacks the template's `Generated-by:` phrase. The note about the open #58979 is stale, since it is merged and in this PR's base. The merge script copies the description into the commit message. @peter-toth Thanks for the re-check! Addressed these. For 4. Fixed the numbering, added Generated-by:, removed the stale #58979 note. -- 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]
