This is an automated email from the ASF dual-hosted git repository.
alamb pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow-datafusion.git
The following commit(s) were added to refs/heads/master by this push:
new 2457ce471 Minor: Avoid cloning as many `Ident` during SQL planning
(#4534)
2457ce471 is described below
commit 2457ce471997c19022af908784fd64a25a99d68e
Author: Andrew Lamb <[email protected]>
AuthorDate: Mon Dec 12 09:41:00 2022 -0500
Minor: Avoid cloning as many `Ident` during SQL planning (#4534)
* Minor: Avoid cloning some Identifiers during planning
* Remove another copy
---
datafusion/expr/src/logical_plan/plan.rs | 10 ++++++---
datafusion/sql/src/planner.rs | 36 +++++++++++++++-----------------
datafusion/sql/src/utils.rs | 10 +--------
3 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/datafusion/expr/src/logical_plan/plan.rs
b/datafusion/expr/src/logical_plan/plan.rs
index 43e615e14..afb178505 100644
--- a/datafusion/expr/src/logical_plan/plan.rs
+++ b/datafusion/expr/src/logical_plan/plan.rs
@@ -1300,13 +1300,17 @@ pub struct SubqueryAlias {
}
impl SubqueryAlias {
- pub fn try_new(plan: LogicalPlan, alias: &str) ->
datafusion_common::Result<Self> {
+ pub fn try_new(
+ plan: LogicalPlan,
+ alias: impl Into<String>,
+ ) -> datafusion_common::Result<Self> {
+ let alias = alias.into();
let schema: Schema = plan.schema().as_ref().clone().into();
let schema =
- DFSchemaRef::new(DFSchema::try_from_qualified_schema(alias,
&schema)?);
+ DFSchemaRef::new(DFSchema::try_from_qualified_schema(&alias,
&schema)?);
Ok(SubqueryAlias {
input: Arc::new(plan),
- alias: alias.to_string(),
+ alias,
schema,
})
}
diff --git a/datafusion/sql/src/planner.rs b/datafusion/sql/src/planner.rs
index 6f54e5803..45eee8e42 100644
--- a/datafusion/sql/src/planner.rs
+++ b/datafusion/sql/src/planner.rs
@@ -72,9 +72,7 @@ use datafusion_expr::{
};
use crate::parser::{CreateExternalTable, DescribeTable, Statement as
DFStatement};
-use crate::utils::{
- make_decimal_type, normalize_ident, normalize_ident_owned, resolve_columns,
-};
+use crate::utils::{make_decimal_type, normalize_ident, resolve_columns};
use super::{
parser::DFParser,
@@ -296,7 +294,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
..
} if with_options.is_empty() => {
let mut plan = self.query_to_plan(*query, &mut
PlannerContext::new())?;
- plan = self.apply_expr_alias(plan, &columns)?;
+ plan = self.apply_expr_alias(plan, columns)?;
Ok(LogicalPlan::CreateView(CreateView {
name: object_name_to_table_reference(name)?,
@@ -484,7 +482,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
for cte in with.cte_tables {
// A `WITH` block can't use the same name more than once
- let cte_name = normalize_ident(&cte.alias.name);
+ let cte_name = normalize_ident(cte.alias.name.clone());
if planner_context.ctes.contains_key(&cte_name) {
return Err(DataFusionError::SQL(ParserError(format!(
"WITH query name {:?} specified more than once",
@@ -689,7 +687,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
.iter()
.any(|x| x.option == ColumnOption::Null);
fields.push(Field::new(
- &normalize_ident(&column.name),
+ &normalize_ident(column.name),
data_type,
allow_null,
));
@@ -894,7 +892,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
JoinConstraint::Using(idents) => {
let keys: Vec<Column> = idents
.into_iter()
- .map(|x| Column::from_name(normalize_ident(&x)))
+ .map(|x| Column::from_name(normalize_ident(x)))
.collect();
LogicalPlanBuilder::from(left)
.join_using(&right, join_type, keys)?
@@ -977,16 +975,16 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
) -> Result<LogicalPlan> {
let apply_name_plan =
LogicalPlan::SubqueryAlias(SubqueryAlias::try_new(
plan,
- &normalize_ident(&alias.name),
+ normalize_ident(alias.name),
)?);
- self.apply_expr_alias(apply_name_plan, &alias.columns)
+ self.apply_expr_alias(apply_name_plan, alias.columns)
}
fn apply_expr_alias(
&self,
plan: LogicalPlan,
- idents: &Vec<Ident>,
+ idents: Vec<Ident>,
) -> Result<LogicalPlan> {
if idents.is_empty() {
Ok(plan)
@@ -999,7 +997,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
} else {
let fields = plan.schema().fields().clone();
LogicalPlanBuilder::from(plan)
- .project(fields.iter().zip(idents.iter()).map(|(field, ident)|
{
+ .project(fields.iter().zip(idents.into_iter()).map(|(field,
ident)| {
col(field.name()).alias(normalize_ident(ident))
}))?
.build()
@@ -1609,7 +1607,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
from_schema,
&[select_expr.clone()],
)?;
- let expr = Alias(Box::new(select_expr),
normalize_ident(&alias));
+ let expr = Alias(Box::new(select_expr),
normalize_ident(alias));
Ok(vec![normalize_col(expr, plan)?])
}
SelectItem::Wildcard(options) => {
@@ -1960,13 +1958,13 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
Ok(Expr::Column(Column {
relation: None,
- name: normalize_ident(&id),
+ name: normalize_ident(id),
}))
}
}
- SQLExpr::MapAccess { ref column, keys } => {
- if let SQLExpr::Identifier(ref id) = column.as_ref() {
+ SQLExpr::MapAccess { column, keys } => {
+ if let SQLExpr::Identifier(id) = *column {
plan_indexed(col(&normalize_ident(id)), keys)
} else {
Err(DataFusionError::NotImplemented(format!(
@@ -1983,7 +1981,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
SQLExpr::CompoundIdentifier(ids) => {
if ids[0].value.starts_with('@') {
- let var_names: Vec<_> = ids.into_iter().map(|s|
normalize_ident(&s)).collect();
+ let var_names: Vec<_> =
ids.into_iter().map(normalize_ident).collect();
let ty = self
.schema_provider
.get_variable_type(&var_names)
@@ -2308,7 +2306,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
// (e.g. "foo.bar") for function names yet
function.name.to_string()
} else {
- normalize_ident(&function.name.0[0])
+ normalize_ident(function.name.0[0].clone())
};
// first, check SQL reserved words
@@ -3074,7 +3072,7 @@ fn idents_to_table_reference(idents: Vec<Ident>) ->
Result<OwnedTableReference>
impl IdentTaker {
fn take(&mut self) -> String {
let ident = self.0.pop().expect("no more identifiers");
- normalize_ident_owned(ident)
+ normalize_ident(ident)
}
}
@@ -3117,7 +3115,7 @@ pub fn object_name_to_qualifier(sql_table_name:
&ObjectName) -> String {
.rev()
.zip(columns)
.map(|(ident, column_name)| {
- format!(r#"{} = '{}'"#, column_name, normalize_ident(ident))
+ format!(r#"{} = '{}'"#, column_name,
normalize_ident(ident.clone()))
})
.collect::<Vec<_>>()
.join(" AND ")
diff --git a/datafusion/sql/src/utils.rs b/datafusion/sql/src/utils.rs
index 4b9ae3ae9..f9624b400 100644
--- a/datafusion/sql/src/utils.rs
+++ b/datafusion/sql/src/utils.rs
@@ -539,16 +539,8 @@ pub(crate) fn make_decimal_type(
}
}
-// Normalize an identifier to a lowercase string unless the identifier is
quoted.
-pub(crate) fn normalize_ident(id: &Ident) -> String {
- match id.quote_style {
- Some(_) => id.value.clone(),
- None => id.value.to_ascii_lowercase(),
- }
-}
-
// Normalize an owned identifier to a lowercase string unless the identifier
is quoted.
-pub(crate) fn normalize_ident_owned(id: Ident) -> String {
+pub(crate) fn normalize_ident(id: Ident) -> String {
match id.quote_style {
Some(_) => id.value,
None => id.value.to_ascii_lowercase(),
