alamb commented on code in PR #10913:
URL: https://github.com/apache/datafusion/pull/10913#discussion_r1642622064


##########
datafusion/core/src/physical_optimizer/combine_partial_final_agg.rs:
##########
@@ -144,8 +144,12 @@ fn can_combine(final_agg: GroupExprsRef, partial_agg: 
GroupExprsRef) -> bool {
     let (input_group_by, input_aggr_expr, input_filter_expr) =
         normalize_group_exprs(partial_agg);
 
-    final_group_by.eq(&input_group_by)

Review Comment:
   Thank you for the explanation. 
   
   My concern is that if someone ever did create a plan that didn't have the 
same grouping expression that this condition could apply and thus cause a very 
hard to debug failure.
   
   I think we should at least add some comments to the check explaining the 
assumption (that a two phase grouping must have semantically the same grouping 
keys) to help future readers / developers. Then I think this PR is ok to merge.
   
   
   I also do wonder if we have some pre-existing code to check two expressions 
for equality from different schemas by normalizing them or something, but I 
didn't try and check for it at the moment



-- 
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: github-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to