Skip to content

minor: avoid cloning the SetExpr during planning of SelectInto #10152

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.

Sign up for GitHub

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

Merged
Jefffrey merged 2 commits into from
Apr 21, 2024
Merged

minor: avoid cloning the SetExpr during planning of SelectInto #10152

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67dcfc9
minor: avoid cloning the `SetExpr` during planning of `SelectInto`
jonahgao Apr 20, 2024
f87d121
retry ci
jonahgao Apr 20, 2024
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
37 changes: 18 additions & 19 deletions datafusion/sql/src/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,29 +46,28 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
query: Query,
planner_context: &mut PlannerContext,
) -> Result<LogicalPlan> {
let set_expr = query.body;
let mut set_expr = query.body;
if let Some(with) = query.with {
self.plan_with_clause(with, planner_context)?;
}
let plan = self.set_expr_to_plan(*(set_expr.clone()), planner_context)?;
Copy link
Member Author

@jonahgao jonahgao Apr 20, 2024

Choose a reason for hiding this comment

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

The removed clone is here.

let plan = self.order_by(plan, query.order_by, planner_context)?;
let plan = self.limit(plan, query.offset, query.limit)?;

let plan = match *set_expr {
SetExpr::Select(select) if select.into.is_some() => {
let select_into = select.into.unwrap();
LogicalPlan::Ddl(DdlStatement::CreateMemoryTable(CreateMemoryTable {
name: self.object_name_to_table_reference(select_into.name)?,
constraints: Constraints::empty(),
input: Arc::new(plan),
if_not_exists: false,
or_replace: false,
column_defaults: vec![],
}))
}
_ => plan,
// Take the `SelectInto` for later processing.
let select_into = match set_expr.as_mut() {
SetExpr::Select(select) => select.into.take(),
_ => None,
};

let plan = self.set_expr_to_plan(*set_expr, planner_context)?;
Copy link
Contributor

@Jefffrey Jefffrey Apr 21, 2024

Choose a reason for hiding this comment

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

I've checked and the select.into field isn't used in this function, so should be good.

Though, I wonder if it might be a concern that we take the into before passing into this function, in case for some future use case it might also need the into? 🤔

Copy link
Member Author

@jonahgao jonahgao Apr 21, 2024

Choose a reason for hiding this comment

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

We can just clone the select.into if needed in the future. Currently, I think INTO is more like an independent planning unit, it is only connected with the input plan through data.

let plan = self.order_by(plan, query.order_by, planner_context)?;
let mut plan = self.limit(plan, query.offset, query.limit)?;
if let Some(into) = select_into {
plan = LogicalPlan::Ddl(DdlStatement::CreateMemoryTable(CreateMemoryTable {
name: self.object_name_to_table_reference(into.name)?,
constraints: Constraints::empty(),
input: Arc::new(plan),
if_not_exists: false,
or_replace: false,
column_defaults: vec![],
}))
}
Ok(plan)
}

Expand Down