From 6ad269dfc315ff602da3355351f1d7f78c3c5a8a Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Wed, 24 Jul 2024 10:11:57 -0400 Subject: [PATCH] Minor: avoid copying order by exprs in planner --- datafusion/sql/src/expr/function.rs | 6 +++--- datafusion/sql/src/expr/order_by.rs | 10 ++++------ datafusion/sql/src/query.rs | 2 +- datafusion/sql/src/select.rs | 2 +- datafusion/sql/src/statement.rs | 2 +- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs index 4804752d8389..0c4b125e76d0 100644 --- a/datafusion/sql/src/expr/function.rs +++ b/datafusion/sql/src/expr/function.rs @@ -274,7 +274,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { .map(|e| self.sql_expr_to_logical_expr(e, schema, planner_context)) .collect::>>()?; let mut order_by = self.order_by_to_sort_expr( - &window.order_by, + window.order_by, schema, planner_context, // Numeric literals in window function ORDER BY are treated as constants @@ -350,7 +350,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { // User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function if let Some(fm) = self.context_provider.get_aggregate_meta(&name) { let order_by = self.order_by_to_sort_expr( - &order_by, + order_by, schema, planner_context, true, @@ -375,7 +375,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { // next, aggregate built-ins if let Ok(fun) = AggregateFunction::from_str(&name) { let order_by = self.order_by_to_sort_expr( - &order_by, + order_by, schema, planner_context, true, diff --git a/datafusion/sql/src/expr/order_by.rs b/datafusion/sql/src/expr/order_by.rs index 4dd81517e958..6010da6fd325 100644 --- a/datafusion/sql/src/expr/order_by.rs +++ b/datafusion/sql/src/expr/order_by.rs @@ -37,7 +37,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { /// If false, interpret numeric literals as constant values. pub(crate) fn order_by_to_sort_expr( &self, - exprs: &[OrderByExpr], + exprs: Vec, input_schema: &DFSchema, planner_context: &mut PlannerContext, literal_to_column: bool, @@ -87,11 +87,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { input_schema.qualified_field(field_index - 1), )) } - e => self.sql_expr_to_logical_expr( - e.clone(), - order_by_schema, - planner_context, - )?, + e => { + self.sql_expr_to_logical_expr(e, order_by_schema, planner_context)? + } }; let asc = asc.unwrap_or(true); expr_vec.push(Expr::Sort(Sort::new( diff --git a/datafusion/sql/src/query.rs b/datafusion/sql/src/query.rs index cbbff19321d8..00560b5c9308 100644 --- a/datafusion/sql/src/query.rs +++ b/datafusion/sql/src/query.rs @@ -59,7 +59,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { other => { let plan = self.set_expr_to_plan(other, planner_context)?; let order_by_rex = self.order_by_to_sort_expr( - &query.order_by, + query.order_by, plan.schema(), planner_context, true, diff --git a/datafusion/sql/src/select.rs b/datafusion/sql/src/select.rs index fc46c3a841b5..9b105117af15 100644 --- a/datafusion/sql/src/select.rs +++ b/datafusion/sql/src/select.rs @@ -101,7 +101,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { // Order-by expressions prioritize referencing columns from the select list, // then from the FROM clause. let order_by_rex = self.order_by_to_sort_expr( - &order_by, + order_by, projected_plan.schema().as_ref(), planner_context, true, diff --git a/datafusion/sql/src/statement.rs b/datafusion/sql/src/statement.rs index 8eb4113f80a6..67107bae0202 100644 --- a/datafusion/sql/src/statement.rs +++ b/datafusion/sql/src/statement.rs @@ -967,7 +967,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { for expr in order_exprs { // Convert each OrderByExpr to a SortExpr: let expr_vec = - self.order_by_to_sort_expr(&expr, schema, planner_context, true, None)?; + self.order_by_to_sort_expr(expr, schema, planner_context, true, None)?; // Verify that columns of all SortExprs exist in the schema: for expr in expr_vec.iter() { for column in expr.column_refs().iter() {