NGA-TRAN commented on code in PR #4490:
URL: https://github.com/apache/arrow-datafusion/pull/4490#discussion_r1043452152
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1360,6 +1370,18 @@ pub struct CreateExternalTable {
pub options: HashMap<String, String>,
}
+/// Prepare a statement but do not execute it. Prepare statements can have 0
or more
+/// `Expr::Placeholder` expressions that are filled in during execution
+#[derive(Clone)]
+pub struct Prepare {
+ /// The name of the statement
+ pub name: String,
+ /// Data types of the parameters ([`Expr::Placeholder`])
+ pub data_types: Vec<DataType>,
+ /// The logical plan of the statements
+ pub input: Arc<LogicalPlan>,
+}
+
Review Comment:
The data types of the placeholders are from the data types of the Vec here
so they match. We do check if the Vec contains enough params, too. However,
there are flexibility:
1. The length of the Vec can be longer than the number of the placeholders
which will be fine and we have tests for this.
2. The data type of the Vec can be anything and we will use them for the
placeholders, we do not check if the data types are compatible with the
variables in the expression because: (i) we allow data type casting, and (2)
when we reach the placeholders, we no longer have the context which
variable/column/expression the place holder is used for. We do not want to add
more context to backtrack which will cost compile time as well as complicated
implementation.
However, I am working on
https://github.com/apache/arrow-datafusion/issues/4550 that convert Prepare
Logical Plan to a logical plan with all placeholders replaced with actual
values. There, I will throw error if the data types provided do not work. We
follow the same behavior of Postgres
##########
datafusion/sql/src/planner.rs:
##########
@@ -1818,6 +1867,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
Ok(Expr::Literal(ScalarValue::Null))
}
SQLExpr::Value(Value::Boolean(n)) => Ok(lit(n)),
+ SQLExpr::Value(Value::Placeholder(param)) => {
+ Self::create_placeholder_expr(param,
param_data_types)
+ }
Review Comment:
I do not get the question but I think my answer for this
https://github.com/apache/arrow-datafusion/pull/4490/files#r1043122807 probably
answer this question, too
##########
datafusion/expr/src/utils.rs:
##########
@@ -579,6 +580,13 @@ pub fn from_plan(
Ok(plan.clone())
}
+ LogicalPlan::Prepare(Prepare {
+ name, data_types, ..
+ }) => Ok(LogicalPlan::Prepare(Prepare {
+ name: name.clone(),
+ data_types: data_types.clone(),
+ input: Arc::new(inputs[0].clone()),
+ })),
Review Comment:
With the code we implement, the answer is no unless there are bugs
##########
datafusion/sql/src/planner.rs:
##########
@@ -1857,6 +1909,41 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
LogicalPlanBuilder::values(values)?.build()
}
+ fn create_placeholder_expr(
+ param: String,
+ param_data_types: &[DataType],
+ ) -> Result<Expr> {
+ // Parse the placeholder as a number because it is the only support
from sqlparser and postgres
Review Comment:
Will do
--
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]
