HairstonE commented on PR #26356: URL: https://github.com/apache/datafusion/pull/26356#issuecomment-6279022137
Hey @adriangb, thanks for pinging me on this. I have one question and I don't think it blocks this from being approved. On line 77, `aggregate.group_expr.iter().any(|expr| matches!(expr, Expr::GroupingSet(_)))` Here we are checking to see if any of the aggregate group expressions are GroupingSets. Is this redundant with the first condition? I think a grouping set can only ever be the sole group expression, which means the first condition would cause this to bail out here anyways. I've gone back and forth on this enough that I'm not sure anymore if it's redundant or guarding against a rarer case. Let me know what you think. -- 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]
