ulysses-you commented on PR #57276: URL: https://github.com/apache/spark/pull/57276#issuecomment-4998434195
Thanks @cloud-fan and @sunchao for the thorough reviews! I addressed all the comments in ada1570: - **cloud-fan (nit)**: fixed the broken Scaladoc on `isPartialAgg`. - **cloud-fan (regression test)**: added `SPARK-43317: Combined sort aggregate keeps the FILTER clause` in `ReplaceHashWithSortAggSuite`, which runs `count(*) FILTER (WHERE ...)` over a `SHUFFLE_MERGE` join with `replaceHashWithSortAgg=true` and asserts a single `Complete` sort aggregate plus correct results, guarding the exact FILTER-drop bug end-to-end. - **cloud-fan (extra coverage)**: added a `count(distinct)` + FILTER case and an object-hash variant of the non-adjacency guard. Note: the `count(distinct)` + FILTER rewrite (Expand + group id) inserts an Exchange between the partial and final aggregate, so they are not adjacent and the rule correctly does not combine them -- the test asserts non-combine plus correct results. - **sunchao (P2a)**: the sort case now uses `max` over a string column so it is genuinely planned as `SortAggregateExec`, with concrete-type assertions before/after. - **sunchao (P2b)**: added a direct rule-level test that keeps the partial/final pair adjacent but mismatches the grouping expressions. PTAL, thanks again! -- 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]
