You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/09/22 15:39:20 UTC
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7622: Group By All
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org