Blizzara commented on code in PR #14553:
URL: https://github.com/apache/datafusion/pull/14553#discussion_r1952853782
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1090,11 +1090,31 @@ impl LogicalPlanBuilder {
group_expr: impl IntoIterator<Item = impl Into<Expr>>,
aggr_expr: impl IntoIterator<Item = impl Into<Expr>>,
) -> Result<Self> {
- let group_expr = normalize_cols(group_expr, &self.plan)?;
+ self._aggregate(group_expr, aggr_expr, true)
+ }
+
+ pub fn aggregate_without_implicit_group_by_exprs(
+ self,
+ group_expr: impl IntoIterator<Item = impl Into<Expr>>,
+ aggr_expr: impl IntoIterator<Item = impl Into<Expr>>,
+ ) -> Result<Self> {
+ self._aggregate(group_expr, aggr_expr, false)
+ }
+
+ fn _aggregate(
Review Comment:
I don't think there's need for `_` since the function is already private (by
virtue of not being `pub fn`). Something like `aggregate_inner` I think is used
quite a lot.
Alternatively, given the logicalplanbuilder for aggregate doesn't do that
much, we could also just inline it into the substrait consumer. That way it's
not changing the LogicalPlanBuilder api, which might be easier.
Or maybe this whole `add_group_by_exprs_from_dependencies` thing should move
from the plan builder into the analyzer/optimizer? Intuitively it feels like
the constructed logical plan shouldn't do this kind of magic, but the
analyzer/optimizer can if it makes things faster to execute. But that might be
a bigger undertaking, so I'd be quite fine with this PR or the alternative
above first.
--
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]
