alamb commented on code in PR #7040:
URL: https://github.com/apache/arrow-datafusion/pull/7040#discussion_r1276146136
##########
datafusion/sql/src/select.rs:
##########
@@ -431,6 +432,37 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
group_by_exprs: Vec<Expr>,
aggr_exprs: Vec<Expr>,
) -> Result<(LogicalPlan, Vec<Expr>, Option<Expr>)> {
+ let schema = input.schema();
Review Comment:
> I thought it is a bit weird for aggregate to receive select_expr argument.
Yes I agree that sounds weird. I still don't understand why the functional
dependency can't be computed from the plan itself (e.g. from the annotations on
DFSchema and then the structure of the LogicalPlan::Aggregate /
LogicalPlan::Project, etc)
If the logic is in the sql planner, that implies the calculation relies on
information that is not encoded in the plan itself (and thus may be lost in
subsequent optimizer passes, for example)
> After this PR merges, I can file another PR, that implements this change.
Then we can decide which implementation is better. In any case, I have moved
groupby update logic to its own function to not clutter this method.
Sounds good
--
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]
