alamb commented on code in PR #4490:
URL: https://github.com/apache/arrow-datafusion/pull/4490#discussion_r1038779386
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -108,6 +108,8 @@ pub enum LogicalPlan {
Distinct(Distinct),
/// Set a Variable
SetVariable(SetVariable),
+ /// Prepare a statement
+ Prepare(Prepare),
Review Comment:
Is the plan to add a `Execute` statement as a follow on PR?
##########
datafusion/sql/src/planner.rs:
##########
@@ -5992,6 +6011,20 @@ mod tests {
quick_test(sql, expected);
}
+ // TODO: will ad more tests to cover maby other cases
+ #[test]
+ fn test_prepare_statement_to_plan() {
+ let sql = "PREPARE my_plan(INT) AS SELECT id, age FROM person WHERE
age = $1";
+ //let statements = DFParser::parse_sql(sql).unwrap();
+
+ let expected = "Prepare: \"my_plan\" [Int32] \
+ \n Projection: person.id, person.age\
+ \n Filter: person.age = $1\
+ \n TableScan: person";
+
+ quick_test(sql, expected);
+ }
+
Review Comment:
The other thing I don't see is code to figure out the placeholders and their
types
##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -344,6 +344,9 @@ 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(
Review Comment:
👍
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1358,6 +1368,17 @@ pub struct CreateExternalTable {
pub options: HashMap<String, String>,
}
+/// Prepare a statement
Review Comment:
```suggestion
/// Prepare a statement but do not execute it. Prepared statements
/// Can have 0 or more `Expr::Placeholder` expressions that are filled in
/// during execution.
```
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -119,6 +121,7 @@ impl LogicalPlanBuilder {
/// The column names are not specified by the SQL standard and different
database systems do it differently,
/// so it's usually better to override the default names with a table
alias list.
pub fn values(mut values: Vec<Vec<Expr>>) -> Result<Self> {
+ // todo: hanlde for Placeholder expr
Review Comment:
yeah, in general the Values statement should probably be cleaned up /
generalized. I can try to help with this next week if that is useful
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -198,7 +199,8 @@ impl ExprSchemable for Expr {
| Expr::IsNotTrue(_)
| Expr::IsNotFalse(_)
| Expr::IsNotUnknown(_)
- | Expr::Exists { .. } => Ok(false),
+ | Expr::Exists { .. }
+ | Expr::Placeholder(_) => Ok(false), // todo: Placeholder should
return false?
Review Comment:
I think since we don't know if the placeholder will be null, it should
return true to be safe
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1358,6 +1368,17 @@ pub struct CreateExternalTable {
pub options: HashMap<String, String>,
}
+/// Prepare a statement
+#[derive(Clone)]
+pub struct Prepare {
+ /// The name of the statement
+ pub name: String,
+ /// Data types of the parameters
Review Comment:
```suggestion
/// Data types of the parameters ([`Expr::PlaceHolder`])
```
##########
datafusion/proto/src/generated/datafusion.rs:
##########
@@ -0,0 +1,1512 @@
+#[derive(Clone, PartialEq, ::prost::Message)]
Review Comment:
Correct -- it should not be part of the PR
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
