Jefffrey commented on code in PR #20169:
URL: https://github.com/apache/datafusion/pull/20169#discussion_r2802746686
##########
datafusion/physical-expr/src/planner.rs:
##########
@@ -288,16 +288,28 @@ pub fn create_physical_expr(
};
Ok(expressions::case(expr, when_then_expr, else_expr)?)
}
- Expr::Cast(Cast { expr, data_type }) => expressions::cast(
- create_physical_expr(expr, input_dfschema, execution_props)?,
- input_schema,
- data_type.clone(),
- ),
- Expr::TryCast(TryCast { expr, data_type }) => expressions::try_cast(
- create_physical_expr(expr, input_dfschema, execution_props)?,
- input_schema,
- data_type.clone(),
- ),
+ Expr::Cast(Cast { expr, data_type }) => {
+ let mut expr = create_physical_expr(expr, input_dfschema,
execution_props)?;
+ if let Some(placeholder) =
expr.as_any().downcast_ref::<PlaceholderExpr>()
+ && placeholder.field.is_none()
+ {
+ expr = expressions::placeholder(&placeholder.id,
data_type.clone());
+ }
+
+ expressions::cast(expr, input_schema, data_type.clone())
+ }
+ Expr::TryCast(TryCast { expr, data_type }) => {
+ let mut expr = create_physical_expr(expr, input_dfschema,
execution_props)?;
+ if let Some(placeholder) =
expr.as_any().downcast_ref::<PlaceholderExpr>()
+ && placeholder.field.is_none()
+ {
+ // To maintain try_cast behavior, we initially resolve the
placeholder with the
Review Comment:
This seems a bit hacky 🤔
What does it mean to maintain the behaviour?
##########
datafusion/physical-expr/src/expressions/placeholder.rs:
##########
@@ -0,0 +1,126 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Placeholder expression.
+
+use std::{
+ any::Any,
+ fmt::{self, Formatter},
+ sync::Arc,
+};
+
+use arrow::{
+ array::RecordBatch,
+ datatypes::{DataType, Field, FieldRef, Schema},
+};
+use datafusion_common::{
+ DataFusionError, Result, exec_datafusion_err, tree_node::TreeNode,
+};
+use datafusion_expr::ColumnarValue;
+use datafusion_physical_expr_common::physical_expr::PhysicalExpr;
+use std::hash::Hash;
+
+/// Physical expression representing a placeholder parameter (e.g., $1, $2, or
named parameters) in
+/// the physical plan.
+///
+/// This expression serves as a placeholder that will be resolved to a literal
value during
+/// execution. It should not be evaluated directly.
+#[derive(Debug, PartialEq, Eq, Hash)]
+pub struct PlaceholderExpr {
+ /// Placeholder id, e.g. $1 or $a.
+ pub id: String,
+ /// Derived from expression where placeholder is met.
+ pub field: Option<FieldRef>,
+}
+
+impl PlaceholderExpr {
+ /// Create a new placeholder expression.
Review Comment:
Personally I find it a bit weird to see `new`, `new_with_X`, `new_without_Y`
in succession as constructor methods; would it be better to have as `new`,
`new_with_datatype`, `new_with_field` so they are strictly additive
semantically or is the `new` (with datatype) common enough to be the default
name?
##########
datafusion/physical-expr/src/expressions/placeholder.rs:
##########
@@ -0,0 +1,126 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Placeholder expression.
+
+use std::{
+ any::Any,
+ fmt::{self, Formatter},
+ sync::Arc,
+};
+
+use arrow::{
+ array::RecordBatch,
+ datatypes::{DataType, Field, FieldRef, Schema},
+};
+use datafusion_common::{
+ DataFusionError, Result, exec_datafusion_err, tree_node::TreeNode,
+};
+use datafusion_expr::ColumnarValue;
+use datafusion_physical_expr_common::physical_expr::PhysicalExpr;
+use std::hash::Hash;
+
+/// Physical expression representing a placeholder parameter (e.g., $1, $2, or
named parameters) in
+/// the physical plan.
+///
+/// This expression serves as a placeholder that will be resolved to a literal
value during
+/// execution. It should not be evaluated directly.
+#[derive(Debug, PartialEq, Eq, Hash)]
+pub struct PlaceholderExpr {
+ /// Placeholder id, e.g. $1 or $a.
+ pub id: String,
+ /// Derived from expression where placeholder is met.
+ pub field: Option<FieldRef>,
+}
+
+impl PlaceholderExpr {
+ /// Create a new placeholder expression.
+ pub fn new(id: String, data_type: DataType) -> Self {
+ let field = Arc::new(Field::new("", data_type, true));
+ Self::new_with_field(id, field)
+ }
+
+ /// Create a new placeholders expression from a field.
+ pub fn new_with_field(id: String, field: FieldRef) -> Self {
+ Self {
+ id,
+ field: Some(field),
+ }
+ }
+
+ /// Create a new placeholder expression without a specified data type.
+ pub fn new_without_data_type(id: String) -> Self {
+ Self { id, field: None }
+ }
+
+ fn execution_error(&self) -> DataFusionError {
+ exec_datafusion_err!(
+ "Placeholder '{}' was not provided a value for execution.",
+ self.id
+ )
+ }
+}
+
+/// Create a placeholder expression.
+pub fn placeholder<I: Into<String>>(id: I, data_type: DataType) -> Arc<dyn
PhysicalExpr> {
+ Arc::new(PlaceholderExpr::new(id.into(), data_type))
+}
+
+/// Returns `true` if expression has placeholders.
+pub fn has_placeholders(expr: &Arc<dyn PhysicalExpr>) -> bool {
+ expr.exists(|e| Ok(e.as_any().is::<PlaceholderExpr>()))
+ .expect("do not return errors")
+}
+
+impl fmt::Display for PlaceholderExpr {
+ fn fmt(&self, f: &mut Formatter) -> fmt::Result {
+ write!(f, "{}", self.id)
+ }
+}
+
+impl PhysicalExpr for PlaceholderExpr {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn return_field(&self, _input_schema: &Schema) -> Result<FieldRef> {
+ self.field
+ .as_ref()
+ .map(Arc::clone)
+ .ok_or_else(|| self.execution_error())
Review Comment:
nit: this error message would complain about not having value but we're
looking at field here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
