ulysses-you commented on PR #57363: URL: https://github.com/apache/spark/pull/57363#issuecomment-5029517899
Thanks @viirya for the thorough trace — agreed on both fixes. Re: `transformDown` -> `transformUp`: good call, I've added a bullet to the PR description explaining it. The reasoning matches what you laid out: `HashAggregateExec` does not expose a grouping-key `outputOrdering` (`Nil`), while `SortAggregateExec` is order-preserving on the grouping keys. With `transformDown` an outer aggregate inspected before its inner partial aggregate sees the partial's `Nil` ordering and is not replaced; with `transformUp` the partial is replaced first, so its sort-aggregate output ordering satisfies the outer aggregate, which is then also replaced. Each replacement is still gated by the `orderingSatisfies` check, so bottom-up can only enable more valid replacements, never wrong ones. The "replace partial and final hash aggregate together with sort aggregate" case in `ReplaceHashWithSortAggSuite` (SMJ -> partial -> final, expecting both replaced) covers it. -- 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]
