alamb commented on code in PR #14553:
URL: https://github.com/apache/datafusion/pull/14553#discussion_r1956303132
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1089,12 +1089,33 @@ impl LogicalPlanBuilder {
self,
group_expr: impl IntoIterator<Item = impl Into<Expr>>,
aggr_expr: impl IntoIterator<Item = impl Into<Expr>>,
+ ) -> Result<Self> {
+ self.aggregate_inner(group_expr, aggr_expr, true)
+ }
+
+ pub fn aggregate_without_implicit_group_by_exprs(
Review Comment:
Yeah I agree adding
`LogicalPlanBuilder::aggregate_without_implicit_group_by_exprs` is not good
(especially without documentation explaining the difference)
What I suggest we do (perhaps as a different PR) is to add a flag to the
builder to control this behavior
```rust
struct LogicalPlanBuilder {
...
/// Should the plan builder add implicit group bys to the plan based on
constraints
add_implicit_group_by_exprs: bool,
}
```
Then when that behavior is needed (in the sql planner) it could be enabled
like
```rust
input
.with_add_implicit_group_by_exprs(true) // new method to see the
flag
.aggregate(group_exprs, aggr_exprs)?
.build()
```
Is this something you would be willing to try @anlinc or @Blizzara ?
--
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]
