mohitgurav20 commented on PR #26000: URL: https://github.com/apache/datafusion/pull/26000#issuecomment-6137693512
Thanks for running the benchmark and flagging this @mkleen. I analyzed the benchmark run in detail: Every single regressed query (Q4, Q8, Q10, Q11, Q34, Q35, Q37, Q40, Q41) is a `COUNT(DISTINCT "UserID")` query where `"UserID"` is `Int64`. Because `count.groups_accumulator_supported_for_types(&[Int64], true)` returns `Some(true)`, the unconditional `rewrite_pays` check prevented `SingleDistinctToGroupBy` from firing on lone distinct queries like `SELECT COUNT(DISTINCT UserID) FROM hits`. Without the two-phase rewrite to `GROUP BY`, the engine cannot leverage `RepartitionExec: partitioning=Hash(...)` to parallelize deduplication across partitions, and falls back to evaluating distincts serially through the accumulator. That explains the 2.57x regression on Q4 (248 ms -> 640 ms) and the slowdowns on Q10 and Q11. This confirms that the two-phase rewrite is a critical parallelization optimization for lone distinct queries, even when a specialized `GroupsAccumulator` exists. To address this cleanly: Lone distinct queries (where all aggregates in the plan are DISTINCT on the same expression, with no companion non-distinct aggregates) should always be rewritten to preserve parallel hash aggregation. The heuristic gating should only apply when non-distinct companion aggregates (like `count(*)` or `sum(y)`) are present. Does that direction sound good to you? If so, I'll update the rule accordingly so we preserve ClickBench performance. -- 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]
