From 488c55a36fb40a2db77266f3f6f24648ab828d62 Mon Sep 17 00:00:00 2001 From: jayzhan211 Date: Mon, 18 Mar 2024 08:39:23 +0800 Subject: [PATCH 1/3] remove clone Signed-off-by: jayzhan211 --- datafusion/expr/src/utils.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index dfd90e470965..1fb70111db81 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -737,7 +737,7 @@ fn agg_cols(agg: &Aggregate) -> Vec { .collect() } -fn exprlist_to_fields_aggregate(exprs: &[Expr], agg: &Aggregate) -> Result> { +fn exprlist_to_fields_aggregate(exprs: &[&Expr], agg: &Aggregate) -> Result> { let agg_cols = agg_cols(agg); let mut fields = vec![]; for expr in exprs { @@ -757,13 +757,13 @@ pub fn exprlist_to_fields<'a>( expr: impl IntoIterator, plan: &LogicalPlan, ) -> Result> { - let exprs: Vec = expr.into_iter().cloned().collect(); + let exprs = expr.into_iter().collect::>(); // when dealing with aggregate plans we cannot simply look in the aggregate output schema // because it will contain columns representing complex expressions (such a column named // `GROUPING(person.state)` so in order to resolve `person.state` in this case we need to // look at the input to the aggregate instead. let fields = match plan { - LogicalPlan::Aggregate(agg) => Some(exprlist_to_fields_aggregate(&exprs, agg)), + LogicalPlan::Aggregate(agg) => Some(exprlist_to_fields_aggregate(exprs.as_slice(), agg)), _ => None, }; if let Some(fields) = fields { From 7f9bba286ef5359e87f06b7188fd7b8a862eacb4 Mon Sep 17 00:00:00 2001 From: jayzhan211 Date: Mon, 18 Mar 2024 08:52:48 +0800 Subject: [PATCH 2/3] remove lifetime Signed-off-by: jayzhan211 --- datafusion/expr/src/logical_plan/plan.rs | 9 +++++---- datafusion/expr/src/utils.rs | 7 +++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index a3f027d9fdb2..c6f280acb255 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -2031,7 +2031,8 @@ impl Window { let fields = input.schema().fields(); let input_len = fields.len(); let mut window_fields = fields.clone(); - window_fields.extend_from_slice(&exprlist_to_fields(window_expr.iter(), &input)?); + let expr_fields = exprlist_to_fields(window_expr.as_slice(), &input)?; + window_fields.extend_from_slice(expr_fields.as_slice()); let metadata = input.schema().metadata().clone(); // Update functional dependencies for window: @@ -2357,7 +2358,7 @@ impl DistinctOn { let on_expr = normalize_cols(on_expr, input.as_ref())?; let schema = DFSchema::new_with_metadata( - exprlist_to_fields(&select_expr, &input)?, + exprlist_to_fields(select_expr.as_slice(), &input)?, input.schema().metadata().clone(), )?; @@ -2436,7 +2437,7 @@ impl Aggregate { let grouping_expr: Vec = grouping_set_to_exprlist(group_expr.as_slice())?; - let mut fields = exprlist_to_fields(grouping_expr.iter(), &input)?; + let mut fields = exprlist_to_fields(grouping_expr.as_slice(), &input)?; // Even columns that cannot be null will become nullable when used in a grouping set. if is_grouping_set { @@ -2446,7 +2447,7 @@ impl Aggregate { .collect::>(); } - fields.extend(exprlist_to_fields(aggr_expr.iter(), &input)?); + fields.extend(exprlist_to_fields(aggr_expr.as_slice(), &input)?); let schema = DFSchema::new_with_metadata(fields, input.schema().metadata().clone())?; diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index 1fb70111db81..2885a4bcb05a 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -737,7 +737,7 @@ fn agg_cols(agg: &Aggregate) -> Vec { .collect() } -fn exprlist_to_fields_aggregate(exprs: &[&Expr], agg: &Aggregate) -> Result> { +fn exprlist_to_fields_aggregate(exprs: &[Expr], agg: &Aggregate) -> Result> { let agg_cols = agg_cols(agg); let mut fields = vec![]; for expr in exprs { @@ -754,16 +754,15 @@ fn exprlist_to_fields_aggregate(exprs: &[&Expr], agg: &Aggregate) -> Result( - expr: impl IntoIterator, + exprs: &[Expr], plan: &LogicalPlan, ) -> Result> { - let exprs = expr.into_iter().collect::>(); // when dealing with aggregate plans we cannot simply look in the aggregate output schema // because it will contain columns representing complex expressions (such a column named // `GROUPING(person.state)` so in order to resolve `person.state` in this case we need to // look at the input to the aggregate instead. let fields = match plan { - LogicalPlan::Aggregate(agg) => Some(exprlist_to_fields_aggregate(exprs.as_slice(), agg)), + LogicalPlan::Aggregate(agg) => Some(exprlist_to_fields_aggregate(exprs, agg)), _ => None, }; if let Some(fields) = fields { From 366160a081b376d46f5b67ed9ebf62059f9bd6e4 Mon Sep 17 00:00:00 2001 From: jayzhan211 Date: Mon, 18 Mar 2024 08:52:58 +0800 Subject: [PATCH 3/3] fmt Signed-off-by: jayzhan211 --- datafusion/expr/src/utils.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index 2885a4bcb05a..c7907d0db16a 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -753,10 +753,7 @@ fn exprlist_to_fields_aggregate(exprs: &[Expr], agg: &Aggregate) -> Result( - exprs: &[Expr], - plan: &LogicalPlan, -) -> Result> { +pub fn exprlist_to_fields(exprs: &[Expr], plan: &LogicalPlan) -> Result> { // when dealing with aggregate plans we cannot simply look in the aggregate output schema // because it will contain columns representing complex expressions (such a column named // `GROUPING(person.state)` so in order to resolve `person.state` in this case we need to