Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor: avoid copying order by exprs in planner #11634

Merged
merged 1 commit into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions datafusion/sql/src/expr/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
.map(|e| self.sql_expr_to_logical_expr(e, schema, planner_context))
.collect::<Result<Vec<_>>>()?;
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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
10 changes: 4 additions & 6 deletions datafusion/sql/src/expr/order_by.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<OrderByExpr>,
Copy link
Contributor Author

@alamb alamb Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internal API changes to take the owned Vec rather than a slice

input_schema: &DFSchema,
planner_context: &mut PlannerContext,
literal_to_column: bool,
Expand Down Expand Up @@ -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(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the clone that is avoided.

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(
Expand Down
2 changes: 1 addition & 1 deletion datafusion/sql/src/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion datafusion/sql/src/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion datafusion/sql/src/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Loading