You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by al...@apache.org on 2023/05/09 15:36:07 UTC
[arrow-datafusion] branch main updated: refactor: Expr::PlaceHolder to use a struct (#6304)
This is an automated email from the ASF dual-hosted git repository.
alamb pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-datafusion.git
The following commit(s) were added to refs/heads/main by this push:
new c7f518327f refactor: Expr::PlaceHolder to use a struct (#6304)
c7f518327f is described below
commit c7f518327f5c16349172f65b31a17671f3904bcb
Author: jakevin <ja...@gmail.com>
AuthorDate: Tue May 9 23:36:01 2023 +0800
refactor: Expr::PlaceHolder to use a struct (#6304)
---
datafusion/core/src/datasource/listing/helpers.rs | 2 +-
.../core/src/datasource/listing_table_factory.rs | 2 +-
datafusion/core/src/physical_plan/planner.rs | 2 +-
datafusion/expr/src/expr.rs | 29 +++++++++++++++-------
datafusion/expr/src/expr_schema.rs | 16 +++++++-----
datafusion/expr/src/logical_plan/plan.rs | 22 ++++++++--------
datafusion/expr/src/tree_node/expr.rs | 10 +++++---
datafusion/expr/src/utils.rs | 2 +-
datafusion/optimizer/src/push_down_filter.rs | 2 +-
.../src/simplify_expressions/expr_simplifier.rs | 2 +-
datafusion/proto/src/logical_plan/from_proto.rs | 14 +++++------
datafusion/proto/src/logical_plan/to_proto.rs | 4 +--
datafusion/sql/src/expr/mod.rs | 4 +--
datafusion/sql/src/expr/value.rs | 10 ++++----
datafusion/sql/src/statement.rs | 13 +++++-----
datafusion/sql/src/utils.rs | 10 ++++----
16 files changed, 80 insertions(+), 64 deletions(-)
diff --git a/datafusion/core/src/datasource/listing/helpers.rs b/datafusion/core/src/datasource/listing/helpers.rs
index 427940d4b7..6032c49885 100644
--- a/datafusion/core/src/datasource/listing/helpers.rs
+++ b/datafusion/core/src/datasource/listing/helpers.rs
@@ -127,7 +127,7 @@ pub fn expr_applicable_for_cols(col_names: &[String], expr: &Expr) -> bool {
| Expr::WindowFunction { .. }
| Expr::Wildcard
| Expr::QualifiedWildcard { .. }
- | Expr::Placeholder { .. } => {
+ | Expr::Placeholder(_) => {
is_applicable = false;
VisitRecursion::Stop
}
diff --git a/datafusion/core/src/datasource/listing_table_factory.rs b/datafusion/core/src/datasource/listing_table_factory.rs
index d61235445a..44595e5122 100644
--- a/datafusion/core/src/datasource/listing_table_factory.rs
+++ b/datafusion/core/src/datasource/listing_table_factory.rs
@@ -169,7 +169,7 @@ impl TableProviderFactory for ListingTableFactory {
fn get_extension(path: &str) -> String {
let res = Path::new(path).extension().and_then(|ext| ext.to_str());
match res {
- Some(ext) => format!(".{}", ext),
+ Some(ext) => format!(".{ext}"),
None => "".to_string(),
}
}
diff --git a/datafusion/core/src/physical_plan/planner.rs b/datafusion/core/src/physical_plan/planner.rs
index 7f99b4e574..cc52739f6a 100644
--- a/datafusion/core/src/physical_plan/planner.rs
+++ b/datafusion/core/src/physical_plan/planner.rs
@@ -345,7 +345,7 @@ fn create_physical_name(e: &Expr, is_first_expr: bool) -> Result<String> {
Expr::QualifiedWildcard { .. } => Err(DataFusionError::Internal(
"Create physical name does not support qualified wildcard".to_string(),
)),
- Expr::Placeholder { .. } => Err(DataFusionError::Internal(
+ Expr::Placeholder(_) => Err(DataFusionError::Internal(
"Create physical name does not support placeholder".to_string(),
)),
Expr::OuterReferenceColumn(_, _) => Err(DataFusionError::Internal(
diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs
index 8b6562285b..55bc71f1ab 100644
--- a/datafusion/expr/src/expr.rs
+++ b/datafusion/expr/src/expr.rs
@@ -177,12 +177,7 @@ pub enum Expr {
GroupingSet(GroupingSet),
/// A place holder for parameters in a prepared statement
/// (e.g. `$foo` or `$1`)
- Placeholder {
- /// The identifier of the parameter (e.g, $1 or $foo)
- id: String,
- /// The type the parameter will be filled in with
- data_type: Option<DataType>,
- },
+ Placeholder(Placeholder),
/// A place holder which hold a reference to a qualified field
/// in the outer query, used for correlated sub queries.
OuterReferenceColumn(DataType, Column),
@@ -562,6 +557,22 @@ impl InSubquery {
}
}
+/// Placeholder
+#[derive(Clone, PartialEq, Eq, Hash)]
+pub struct Placeholder {
+ /// The identifier of the parameter (e.g, $1 or $foo)
+ pub id: String,
+ /// The type the parameter will be filled in with
+ pub data_type: Option<DataType>,
+}
+
+impl Placeholder {
+ /// Create a new Placeholder expression
+ pub fn new(id: String, data_type: Option<DataType>) -> Self {
+ Self { id, data_type }
+ }
+}
+
/// Grouping sets
/// See <https://www.postgresql.org/docs/current/queries-table-expressions.html#QUERIES-GROUPING-SETS>
/// for Postgres definition.
@@ -666,7 +677,7 @@ impl Expr {
Expr::Literal(..) => "Literal",
Expr::Negative(..) => "Negative",
Expr::Not(..) => "Not",
- Expr::Placeholder { .. } => "Placeholder",
+ Expr::Placeholder(_) => "Placeholder",
Expr::QualifiedWildcard { .. } => "QualifiedWildcard",
Expr::ScalarFunction(..) => "ScalarFunction",
Expr::ScalarSubquery { .. } => "ScalarSubquery",
@@ -1172,7 +1183,7 @@ impl fmt::Debug for Expr {
)
}
},
- Expr::Placeholder { id, .. } => write!(f, "{id}"),
+ Expr::Placeholder(Placeholder { id, .. }) => write!(f, "{id}"),
}
}
}
@@ -1457,7 +1468,7 @@ fn create_name(e: &Expr) -> Result<String> {
Expr::QualifiedWildcard { .. } => Err(DataFusionError::Internal(
"Create name does not support qualified wildcard".to_string(),
)),
- Expr::Placeholder { id, .. } => Ok((*id).to_string()),
+ Expr::Placeholder(Placeholder { id, .. }) => Ok((*id).to_string()),
}
}
diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs
index 8b6afdec94..680a5cb784 100644
--- a/datafusion/expr/src/expr_schema.rs
+++ b/datafusion/expr/src/expr_schema.rs
@@ -18,7 +18,7 @@
use super::{Between, Expr, Like};
use crate::expr::{
AggregateFunction, AggregateUDF, BinaryExpr, Cast, GetIndexedField, InList,
- InSubquery, ScalarFunction, ScalarUDF, Sort, TryCast, WindowFunction,
+ InSubquery, Placeholder, ScalarFunction, ScalarUDF, Sort, TryCast, WindowFunction,
};
use crate::field_util::get_indexed_field;
use crate::type_coercion::binary::get_result_type;
@@ -62,7 +62,7 @@ impl ExprSchemable for Expr {
fn get_type<S: ExprSchema>(&self, schema: &S) -> Result<DataType> {
match self {
Expr::Alias(expr, name) => match &**expr {
- Expr::Placeholder { data_type, .. } => match &data_type {
+ Expr::Placeholder(Placeholder { data_type, .. }) => match &data_type {
None => schema.data_type(&Column::from_name(name)).cloned(),
Some(dt) => Ok(dt.clone()),
},
@@ -154,9 +154,13 @@ impl ExprSchemable for Expr {
Expr::Like { .. } | Expr::ILike { .. } | Expr::SimilarTo { .. } => {
Ok(DataType::Boolean)
}
- Expr::Placeholder { data_type, .. } => data_type.clone().ok_or_else(|| {
- DataFusionError::Plan("Placeholder type could not be resolved".to_owned())
- }),
+ Expr::Placeholder(Placeholder { data_type, .. }) => {
+ data_type.clone().ok_or_else(|| {
+ DataFusionError::Plan(
+ "Placeholder type could not be resolved".to_owned(),
+ )
+ })
+ }
Expr::Wildcard => {
// Wildcard do not really have a type and do not appear in projections
Ok(DataType::Null)
@@ -231,7 +235,7 @@ impl ExprSchemable for Expr {
| Expr::IsNotFalse(_)
| Expr::IsNotUnknown(_)
| Expr::Exists { .. }
- | Expr::Placeholder { .. } => Ok(true),
+ | Expr::Placeholder(_) => Ok(true),
Expr::InSubquery(InSubquery { expr, .. }) => expr.nullable(input_schema),
Expr::ScalarSubquery(subquery) => {
Ok(subquery.subquery.schema().field(0).is_nullable())
diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs
index fdd875ca84..d95edb7e7c 100644
--- a/datafusion/expr/src/logical_plan/plan.rs
+++ b/datafusion/expr/src/logical_plan/plan.rs
@@ -15,8 +15,8 @@
// specific language governing permissions and limitations
// under the License.
-use crate::expr::Exists;
use crate::expr::InSubquery;
+use crate::expr::{Exists, Placeholder};
///! Logical plan types
use crate::logical_plan::display::{GraphvizVisitor, IndentVisitor};
use crate::logical_plan::extension::UserDefinedLogicalNode;
@@ -620,7 +620,7 @@ impl LogicalPlan {
self.apply(&mut |plan| {
plan.inspect_expressions(|expr| {
expr.apply(&mut |expr| {
- if let Expr::Placeholder { id, data_type } = expr {
+ if let Expr::Placeholder(Placeholder { id, data_type }) = expr {
let prev = param_types.get(id);
match (prev, data_type) {
(Some(Some(prev)), Some(dt)) => {
@@ -654,7 +654,7 @@ impl LogicalPlan {
) -> Result<Expr> {
expr.transform(&|expr| {
match &expr {
- Expr::Placeholder { id, data_type } => {
+ Expr::Placeholder(Placeholder { id, data_type }) => {
if id.is_empty() || id == "$0" {
return Err(DataFusionError::Plan(
"Empty placeholder id".to_string(),
@@ -2264,10 +2264,10 @@ mod tests {
let plan = table_scan(TableReference::none(), &schema, None)
.unwrap()
- .filter(col("id").eq(Expr::Placeholder {
- id: "".into(),
- data_type: Some(DataType::Int32),
- }))
+ .filter(col("id").eq(Expr::Placeholder(Placeholder::new(
+ "".into(),
+ Some(DataType::Int32),
+ ))))
.unwrap()
.build()
.unwrap();
@@ -2280,10 +2280,10 @@ mod tests {
let plan = table_scan(TableReference::none(), &schema, None)
.unwrap()
- .filter(col("id").eq(Expr::Placeholder {
- id: "$0".into(),
- data_type: Some(DataType::Int32),
- }))
+ .filter(col("id").eq(Expr::Placeholder(Placeholder::new(
+ "$0".into(),
+ Some(DataType::Int32),
+ ))))
.unwrap()
.build()
.unwrap();
diff --git a/datafusion/expr/src/tree_node/expr.rs b/datafusion/expr/src/tree_node/expr.rs
index 7bc9edf4c2..3b8df59dab 100644
--- a/datafusion/expr/src/tree_node/expr.rs
+++ b/datafusion/expr/src/tree_node/expr.rs
@@ -19,8 +19,8 @@
use crate::expr::{
AggregateFunction, AggregateUDF, Between, BinaryExpr, Case, Cast, GetIndexedField,
- GroupingSet, InList, InSubquery, Like, ScalarFunction, ScalarUDF, Sort, TryCast,
- WindowFunction,
+ GroupingSet, InList, InSubquery, Like, Placeholder, ScalarFunction, ScalarUDF, Sort,
+ TryCast, WindowFunction,
};
use crate::Expr;
use datafusion_common::tree_node::VisitRecursion;
@@ -67,7 +67,7 @@ impl TreeNode for Expr {
| Expr::ScalarSubquery(_)
| Expr::Wildcard
| Expr::QualifiedWildcard { .. }
- | Expr::Placeholder { .. } => vec![],
+ | Expr::Placeholder (_) => vec![],
Expr::BinaryExpr(BinaryExpr { left, right, .. }) => {
vec![left.as_ref().clone(), right.as_ref().clone()]
}
@@ -340,7 +340,9 @@ impl TreeNode for Expr {
key,
))
}
- Expr::Placeholder { id, data_type } => Expr::Placeholder { id, data_type },
+ Expr::Placeholder(Placeholder { id, data_type }) => {
+ Expr::Placeholder(Placeholder { id, data_type })
+ }
})
}
}
diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs
index 06f9ccd11c..1d7aa536f9 100644
--- a/datafusion/expr/src/utils.rs
+++ b/datafusion/expr/src/utils.rs
@@ -308,7 +308,7 @@ pub fn expr_to_columns(expr: &Expr, accum: &mut HashSet<Column>) -> Result<()> {
| Expr::Wildcard
| Expr::QualifiedWildcard { .. }
| Expr::GetIndexedField { .. }
- | Expr::Placeholder { .. }
+ | Expr::Placeholder(_)
| Expr::OuterReferenceColumn { .. } => {}
}
Ok(())
diff --git a/datafusion/optimizer/src/push_down_filter.rs b/datafusion/optimizer/src/push_down_filter.rs
index d724c59d0d..175c6a118c 100644
--- a/datafusion/optimizer/src/push_down_filter.rs
+++ b/datafusion/optimizer/src/push_down_filter.rs
@@ -154,7 +154,7 @@ fn can_evaluate_as_join_condition(predicate: &Expr) -> Result<bool> {
predicate.apply(&mut |expr| match expr {
Expr::Column(_)
| Expr::Literal(_)
- | Expr::Placeholder { .. }
+ | Expr::Placeholder(_)
| Expr::ScalarVariable(_, _) => Ok(VisitRecursion::Skip),
Expr::Exists { .. }
| Expr::InSubquery(_)
diff --git a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
index 07391a2d77..d077c9f83a 100644
--- a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
+++ b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
@@ -265,7 +265,7 @@ impl<'a> ConstEvaluator<'a> {
| Expr::GroupingSet(_)
| Expr::Wildcard
| Expr::QualifiedWildcard { .. }
- | Expr::Placeholder { .. } => false,
+ | Expr::Placeholder(_) => false,
Expr::ScalarFunction(ScalarFunction { fun, .. }) => {
Self::volatility_ok(fun.volatility())
}
diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs
index 890baba757..5afd94b5a7 100644
--- a/datafusion/proto/src/logical_plan/from_proto.rs
+++ b/datafusion/proto/src/logical_plan/from_proto.rs
@@ -34,6 +34,7 @@ use datafusion_common::{
Column, DFField, DFSchema, DFSchemaRef, DataFusionError, OwnedTableReference, Result,
ScalarValue,
};
+use datafusion_expr::expr::Placeholder;
use datafusion_expr::{
abs, acos, acosh, array, ascii, asin, asinh, atan, atan2, atanh, bit_length, btrim,
cbrt, ceil, character_length, chr, coalesce, concat_expr, concat_ws_expr, cos, cosh,
@@ -1413,14 +1414,11 @@ pub fn parse_expr(
)))
}
ExprType::Placeholder(PlaceholderNode { id, data_type }) => match data_type {
- None => Ok(Expr::Placeholder {
- id: id.clone(),
- data_type: None,
- }),
- Some(data_type) => Ok(Expr::Placeholder {
- id: id.clone(),
- data_type: Some(data_type.try_into()?),
- }),
+ None => Ok(Expr::Placeholder(Placeholder::new(id.clone(), None))),
+ Some(data_type) => Ok(Expr::Placeholder(Placeholder::new(
+ id.clone(),
+ Some(data_type.try_into()?),
+ ))),
},
}
}
diff --git a/datafusion/proto/src/logical_plan/to_proto.rs b/datafusion/proto/src/logical_plan/to_proto.rs
index 263acd58dc..e881051f27 100644
--- a/datafusion/proto/src/logical_plan/to_proto.rs
+++ b/datafusion/proto/src/logical_plan/to_proto.rs
@@ -37,7 +37,7 @@ use arrow::datatypes::{
use datafusion_common::{Column, DFField, DFSchemaRef, OwnedTableReference, ScalarValue};
use datafusion_expr::expr::{
self, Between, BinaryExpr, Cast, GetIndexedField, GroupingSet, InList, Like,
- ScalarFunction, ScalarUDF, Sort,
+ Placeholder, ScalarFunction, ScalarUDF, Sort,
};
use datafusion_expr::{
logical_plan::PlanType, logical_plan::StringifiedPlan, AggregateFunction,
@@ -944,7 +944,7 @@ impl TryFrom<&Expr> for protobuf::LogicalExprNode {
.collect::<Result<Vec<_>, Self::Error>>()?,
})),
},
- Expr::Placeholder { id, data_type } => {
+ Expr::Placeholder(Placeholder { id, data_type }) => {
let data_type = match data_type {
Some(data_type) => Some(data_type.try_into()?),
None => None,
diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs
index a281e8a985..a120b3400a 100644
--- a/datafusion/sql/src/expr/mod.rs
+++ b/datafusion/sql/src/expr/mod.rs
@@ -30,8 +30,8 @@ use crate::planner::{ContextProvider, PlannerContext, SqlToRel};
use arrow_schema::DataType;
use datafusion_common::tree_node::{Transformed, TreeNode};
use datafusion_common::{Column, DFSchema, DataFusionError, Result, ScalarValue};
-use datafusion_expr::expr::InList;
use datafusion_expr::expr::ScalarFunction;
+use datafusion_expr::expr::{InList, Placeholder};
use datafusion_expr::{
col, expr, lit, AggregateFunction, Between, BinaryExpr, BuiltinScalarFunction, Cast,
Expr, ExprSchemable, GetIndexedField, Like, Operator, TryCast,
@@ -500,7 +500,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
// modifies expr if it is a placeholder with datatype of right
fn rewrite_placeholder(expr: &mut Expr, other: &Expr, schema: &DFSchema) -> Result<()> {
- if let Expr::Placeholder { id: _, data_type } = expr {
+ if let Expr::Placeholder(Placeholder { id: _, data_type }) = expr {
if data_type.is_none() {
let other_dt = other.get_type(schema);
match other_dt {
diff --git a/datafusion/sql/src/expr/value.rs b/datafusion/sql/src/expr/value.rs
index e747bf1b2a..3d959f17ce 100644
--- a/datafusion/sql/src/expr/value.rs
+++ b/datafusion/sql/src/expr/value.rs
@@ -19,7 +19,7 @@ use crate::planner::{ContextProvider, PlannerContext, SqlToRel};
use arrow::compute::kernels::cast_utils::parse_interval_month_day_nano;
use arrow_schema::DataType;
use datafusion_common::{DFSchema, DataFusionError, Result, ScalarValue};
-use datafusion_expr::expr::BinaryExpr;
+use datafusion_expr::expr::{BinaryExpr, Placeholder};
use datafusion_expr::{lit, Expr, Operator};
use log::debug;
use sqlparser::ast::{BinaryOperator, DateTimeField, Expr as SQLExpr, Value};
@@ -114,10 +114,10 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
param, param_type
);
- Ok(Expr::Placeholder {
- id: param,
- data_type: param_type.cloned(),
- })
+ Ok(Expr::Placeholder(Placeholder::new(
+ param,
+ param_type.cloned(),
+ )))
}
pub(super) fn sql_array_literal(
diff --git a/datafusion/sql/src/statement.rs b/datafusion/sql/src/statement.rs
index 779a79e155..16d3c6c793 100644
--- a/datafusion/sql/src/statement.rs
+++ b/datafusion/sql/src/statement.rs
@@ -48,6 +48,7 @@ use sqlparser::ast::{
TableConstraint, TableFactor, TableWithJoins, TransactionMode, UnaryOperator, Value,
};
+use datafusion_expr::expr::Placeholder;
use sqlparser::parser::ParserError::ParserError;
use std::collections::{BTreeMap, HashMap, HashSet};
use std::sync::Arc;
@@ -902,16 +903,16 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
for (col_name, expr) in values.into_iter() {
let expr = self.sql_to_expr(expr, &table_schema, &mut planner_context)?;
let expr = match expr {
- datafusion_expr::Expr::Placeholder {
+ datafusion_expr::Expr::Placeholder(Placeholder {
ref id,
ref data_type,
- } => match data_type {
+ }) => match data_type {
None => {
let dt = table_schema.data_type(&Column::from_name(&col_name))?;
- datafusion_expr::Expr::Placeholder {
- id: id.clone(),
- data_type: Some(dt.clone()),
- }
+ datafusion_expr::Expr::Placeholder(Placeholder::new(
+ id.clone(),
+ Some(dt.clone()),
+ ))
}
Some(_) => expr,
},
diff --git a/datafusion/sql/src/utils.rs b/datafusion/sql/src/utils.rs
index 1bfff37b48..1605433609 100644
--- a/datafusion/sql/src/utils.rs
+++ b/datafusion/sql/src/utils.rs
@@ -23,7 +23,8 @@ use sqlparser::ast::Ident;
use datafusion_common::{DataFusionError, Result, ScalarValue};
use datafusion_expr::expr::{
AggregateFunction, AggregateUDF, Between, BinaryExpr, Case, GetIndexedField,
- GroupingSet, InList, InSubquery, Like, ScalarFunction, ScalarUDF, WindowFunction,
+ GroupingSet, InList, InSubquery, Like, Placeholder, ScalarFunction, ScalarUDF,
+ WindowFunction,
};
use datafusion_expr::expr::{Cast, Sort};
use datafusion_expr::utils::{expr_as_column_expr, find_column_exprs};
@@ -413,10 +414,9 @@ where
)))
}
},
- Expr::Placeholder { id, data_type } => Ok(Expr::Placeholder {
- id: id.clone(),
- data_type: data_type.clone(),
- }),
+ Expr::Placeholder(Placeholder { id, data_type }) => Ok(Expr::Placeholder(
+ Placeholder::new(id.clone(), data_type.clone()),
+ )),
},
}
}