mustafasrepo commented on code in PR #7040:
URL: https://github.com/apache/arrow-datafusion/pull/7040#discussion_r1276074747
##########
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:
In here, we update group by exprs by the expressions that are in target
indices (if they are in the select exprs). With this change, we can use
dependents that are among select exprs, when their determinant is inside group
by expression.
We can do so inside `Aggregate::try_new_with_schema` if we pass
`select_expr` argument to it. I thought it is a bit weird for aggregate to
receive `select_expr` argument. 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.
--
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]
