alamb commented on code in PR #7622:
URL: https://github.com/apache/arrow-datafusion/pull/7622#discussion_r1334545778
##########
datafusion/sql/src/select.rs:
##########
@@ -136,29 +137,49 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let aggr_exprs = find_aggregate_exprs(&aggr_expr_haystack);
// All of the group by expressions
- let group_by_exprs = select
- .group_by
- .into_iter()
- .map(|e| {
- let group_by_expr =
- self.sql_expr_to_logical_expr(e, &combined_schema,
planner_context)?;
- // aliases from the projection can conflict with same-named
expressions in the input
- let mut alias_map = alias_map.clone();
- for f in plan.schema().fields() {
- alias_map.remove(f.name());
- }
- let group_by_expr = resolve_aliases_to_exprs(&group_by_expr,
&alias_map)?;
- let group_by_expr =
- resolve_positions_to_exprs(&group_by_expr, &select_exprs)
- .unwrap_or(group_by_expr);
- let group_by_expr = normalize_col(group_by_expr,
&projected_plan)?;
- self.validate_schema_satisfies_exprs(
- plan.schema(),
- &[group_by_expr.clone()],
- )?;
- Ok(group_by_expr)
- })
- .collect::<Result<Vec<Expr>>>()?;
+ let group_by_exprs = if let GroupByExpr::Expressions(exprs) =
select.group_by {
+ exprs
+ .into_iter()
+ .map(|e| {
+ let group_by_expr = self.sql_expr_to_logical_expr(
+ e,
+ &combined_schema,
+ planner_context,
+ )?;
+ // aliases from the projection can conflict with
same-named expressions in the input
+ let mut alias_map = alias_map.clone();
+ for f in plan.schema().fields() {
+ alias_map.remove(f.name());
+ }
+ let group_by_expr =
+ resolve_aliases_to_exprs(&group_by_expr, &alias_map)?;
+ let group_by_expr =
+ resolve_positions_to_exprs(&group_by_expr,
&select_exprs)
+ .unwrap_or(group_by_expr);
+ let group_by_expr = normalize_col(group_by_expr,
&projected_plan)?;
+ self.validate_schema_satisfies_exprs(
+ plan.schema(),
+ &[group_by_expr.clone()],
+ )?;
+ Ok(group_by_expr)
+ })
+ .collect::<Result<Vec<Expr>>>()?
+ } else {
+ // 'group by all' groups wrt. all select expressions except
'AggregateFunction's.
+ // Filter and collect non-aggregate select expressions
+ select_exprs
Review Comment:
👍 very nice explanation and code
##########
datafusion/sql/src/select.rs:
##########
@@ -136,29 +137,49 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let aggr_exprs = find_aggregate_exprs(&aggr_expr_haystack);
// All of the group by expressions
- let group_by_exprs = select
- .group_by
- .into_iter()
- .map(|e| {
- let group_by_expr =
- self.sql_expr_to_logical_expr(e, &combined_schema,
planner_context)?;
- // aliases from the projection can conflict with same-named
expressions in the input
- let mut alias_map = alias_map.clone();
- for f in plan.schema().fields() {
- alias_map.remove(f.name());
- }
- let group_by_expr = resolve_aliases_to_exprs(&group_by_expr,
&alias_map)?;
- let group_by_expr =
- resolve_positions_to_exprs(&group_by_expr, &select_exprs)
- .unwrap_or(group_by_expr);
- let group_by_expr = normalize_col(group_by_expr,
&projected_plan)?;
- self.validate_schema_satisfies_exprs(
- plan.schema(),
- &[group_by_expr.clone()],
- )?;
- Ok(group_by_expr)
- })
- .collect::<Result<Vec<Expr>>>()?;
+ let group_by_exprs = if let GroupByExpr::Expressions(exprs) =
select.group_by {
+ exprs
+ .into_iter()
+ .map(|e| {
+ let group_by_expr = self.sql_expr_to_logical_expr(
+ e,
+ &combined_schema,
+ planner_context,
+ )?;
+ // aliases from the projection can conflict with
same-named expressions in the input
+ let mut alias_map = alias_map.clone();
+ for f in plan.schema().fields() {
+ alias_map.remove(f.name());
+ }
+ let group_by_expr =
+ resolve_aliases_to_exprs(&group_by_expr, &alias_map)?;
+ let group_by_expr =
+ resolve_positions_to_exprs(&group_by_expr,
&select_exprs)
+ .unwrap_or(group_by_expr);
+ let group_by_expr = normalize_col(group_by_expr,
&projected_plan)?;
+ self.validate_schema_satisfies_exprs(
+ plan.schema(),
+ &[group_by_expr.clone()],
+ )?;
+ Ok(group_by_expr)
+ })
+ .collect::<Result<Vec<Expr>>>()?
+ } else {
+ // 'group by all' groups wrt. all select expressions except
'AggregateFunction's.
+ // Filter and collect non-aggregate select expressions
+ select_exprs
Review Comment:
👍 very nice explanation and code
--
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]
