apache · alamb · May 9, 2023 · May 9, 2023
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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()),
+            )),
         },
     }
 }