vbarua commented on code in PR #14860:
URL: https://github.com/apache/datafusion/pull/14860#discussion_r1970267828
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -63,6 +63,26 @@ use indexmap::IndexSet;
/// Default table name for unnamed table
pub const UNNAMED_TABLE: &str = "?table?";
+/// Options for [`LogicalPlanBuilder`]
+#[derive(Default, Debug, Clone)]
+pub struct LogicalPlanBuilderOptions {
+ /// Flag indicating whether the plan builder should add
+ /// functionally dependent expressions as additional aggregation groupings.
+ add_implicit_group_by_exprs: bool,
Review Comment:
I lean towards having the default behaviour be `false` for this, even if
it's a breaking change, because it makes the builder less surprising IMO.
Specifically, when invoking the builder for an aggregate with a specific set of
grouping expressions, my expectation is that it should produce an aggregate
with those specific grouping expressions. If I wanted additional grouping
expressions, I would have included them.
There's definitely room and value for optimizations like what is going on
here, but I think those need to be opt-in to avoid situation like this were the
plan builder tries to be smart along one specific axis and inadvertently shoots
you in the foot in another. In the past, I think we've leaned towards having
the builder be as straightforward as possible and then handling optimizations
in the optimizer.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
