alamb commented on code in PR #4490:
URL: https://github.com/apache/arrow-datafusion/pull/4490#discussion_r1042650712
##########
datafusion/expr/src/expr.rs:
##########
@@ -244,6 +244,14 @@ pub enum Expr {
/// List of grouping set expressions. Only valid in the context of an
aggregate
/// GROUP BY expression list
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,
Review Comment:
👍
##########
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 becasue it is the only support
from sqlparser and postgres
+ let index = param[1..].parse::<usize>();
+ let idx = match index {
+ Ok(index) => index - 1,
+ Err(_) => {
+ return Err(DataFusionError::Internal(format!(
+ "Invalid placeholder: {}",
Review Comment:
```suggestion
"Invalid placeholder, not a number: {}",
```
##########
datafusion/core/tests/sqllogictests/test_files/prepare.slt:
##########
@@ -0,0 +1,40 @@
+# 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.
+
+##########
+## Prepare Statement Tests
+##########
+
+statement ok
+create table person (id int, first_name varchar, last_name varchar, age int,
state varchar, salary double, birthday timestamp, "😀" int) as values (1,
'jane', 'smith', 20, 'MA', 100000.45, '2000-11-12T00:00:00'::timestamp, 99);
+
+query C rowsort
+select * from person;
+----
+1 jane smith 20 MA 100000.45 2000-11-12T00:00:00.000000000 99
+
+# TODO: support error instead of panicking
+# thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value:
SQL(ParserError("Expected AS, found: SELECT"))',
datafusion/core/tests/sqllogictests/src/main.rs:197:42
+# statement error
+# PREPARE AS SELECT id, age FROM person WHERE age = $foo
+
+# TODO: this statement should work after we support EXECUTE statement and
caching this prepare logical plan somewhere
+# statement ok
+# PREPARE my_plan(STRING, STRING) AS SELECT * FROM (VALUES(1, $1), (2, $2)) AS
t (num, letter);
+
+# And then we may want to add test_prepare_statement* here
+
Review Comment:
Perhaps I can add a subtask to implement it under
https://github.com/apache/arrow-datafusion/issues/4539 -- then in DataFusion we
normally leave a link to the ticket as a comment
##########
datafusion/sql/src/planner.rs:
##########
@@ -16,7 +16,7 @@
// under the License.
//! SQL Query Planner (produces logical plan from SQL AST)
-
+use log::debug;
Review Comment:
👍 I have wanted some debug logging in the planner as well
##########
datafusion/sql/src/planner.rs:
##########
@@ -6144,6 +6247,185 @@ mod tests {
quick_test(sql, expected);
}
+ #[test]
+ #[should_panic(expected = "Invalid placeholder: $foo")]
+ fn test_prepare_statement_to_plan_panic_param_format() {
+ // param is not number following the $ sign
+ // panic due to error returned from the parser
+ let sql = "PREPARE my_plan(INT) AS SELECT id, age FROM person WHERE
age = $foo";
+
+ let expected_plan = "whatever";
+ let expected_dt = "whatever";
+
+ prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+ }
+
+ #[test]
+ #[should_panic(expected = "value: SQL(ParserError(\"Expected AS, found:
SELECT\"))")]
+ fn test_prepare_statement_to_plan_panic_prepare_wrong_syntax() {
+ // param is not number following the $ sign
+ // panic due to error returned from the parser
+ let sql = "PREPARE AS SELECT id, age FROM person WHERE age = $foo";
+
+ let expected_plan = "whatever";
+ let expected_dt = "whatever";
+
+ prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+ }
+
+ #[test]
+ #[should_panic(
+ expected = "value: SchemaError(FieldNotFound { field: Column {
relation: None, name: \"id\" }, valid_fields: Some([]) })"
+ )]
+ fn test_prepare_statement_to_plan_panic_no_relation_and_constant_param() {
+ let sql = "PREPARE my_plan(INT) AS SELECT id + $1";
+
+ let expected_plan = "whatever";
+ let expected_dt = "whatever";
+
+ prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+ }
+
+ #[test]
+ #[should_panic(
+ expected = "value: Internal(\"Placehoder $2 does not exist in the
parameter list: [Int32]\")"
+ )]
+ fn test_prepare_statement_to_plan_panic_no_data_types() {
+ // only provide 1 data type while using 2 params
+ let sql = "PREPARE my_plan(INT) AS SELECT 1 + $1 + $2";
+
+ let expected_plan = "whatever";
+ let expected_dt = "whatever";
+
+ prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+ }
+
+ #[test]
+ fn test_prepare_statement_to_plan_no_param() {
+ // no embedded parameter but still declare it
+ let sql = "PREPARE my_plan(INT) AS SELECT id, age FROM person WHERE
age = 10";
+
+ let expected_plan = "Prepare: \"my_plan\" [Int32] \
+ \n Projection: person.id, person.age\
+ \n Filter: person.age = Int64(10)\
+ \n TableScan: person";
+
+ let expected_dt = "[Int32]";
+
+ prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+
+ /////////////////////////
+ // no embedded parameter and no declare it
+ let sql = "PREPARE my_plan AS SELECT id, age FROM person WHERE age =
10";
+
+ let expected_plan = "Prepare: \"my_plan\" [] \
+ \n Projection: person.id, person.age\
+ \n Filter: person.age = Int64(10)\
+ \n TableScan: person";
+
+ let expected_dt = "[]";
+
+ prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+ }
+
+ #[test]
+ fn test_prepare_statement_to_plan_params_as_constants() {
+ let sql = "PREPARE my_plan(INT) AS SELECT $1";
+
+ let expected_plan = "Prepare: \"my_plan\" [Int32] \
+ \n Projection: $1\n EmptyRelation";
+ let expected_dt = "[Int32]";
+
+ prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+
+ /////////////////////////
+ let sql = "PREPARE my_plan(INT) AS SELECT 1 + $1";
+
+ let expected_plan = "Prepare: \"my_plan\" [Int32] \
+ \n Projection: Int64(1) + $1\n EmptyRelation";
+ let expected_dt = "[Int32]";
+
+ prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+
+ /////////////////////////
+ let sql = "PREPARE my_plan(INT, DOUBLE) AS SELECT 1 + $1 + $2";
+
+ let expected_plan = "Prepare: \"my_plan\" [Int32, Float64] \
+ \n Projection: Int64(1) + $1 + $2\n EmptyRelation";
+ let expected_dt = "[Int32, Float64]";
Review Comment:
👍
##########
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 becasue it is the only support
from sqlparser and postgres
Review Comment:
```suggestion
// Parse the placeholder as a number because it is the only support
from sqlparser and postgres
```
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -279,6 +281,14 @@ impl LogicalPlanBuilder {
)?)))
}
+ pub fn prepare(&self, name: String, data_types: Vec<DataType>) ->
Result<Self> {
Review Comment:
Perhaps we can add a docstring to this method
##########
datafusion/sql/src/planner.rs:
##########
@@ -6144,6 +6247,185 @@ mod tests {
quick_test(sql, expected);
}
+ #[test]
+ #[should_panic(expected = "Invalid placeholder: $foo")]
+ fn test_prepare_statement_to_plan_panic_param_format() {
+ // param is not number following the $ sign
+ // panic due to error returned from the parser
+ let sql = "PREPARE my_plan(INT) AS SELECT id, age FROM person WHERE
age = $foo";
+
+ let expected_plan = "whatever";
+ let expected_dt = "whatever";
+
+ prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+ }
+
+ #[test]
Review Comment:
👍 very nice tests
##########
datafusion/core/tests/sqllogictests/test_files/prepare.slt:
##########
@@ -0,0 +1,40 @@
+# 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.
+
+##########
+## Prepare Statement Tests
+##########
+
+statement ok
+create table person (id int, first_name varchar, last_name varchar, age int,
state varchar, salary double, birthday timestamp, "😀" int) as values (1,
'jane', 'smith', 20, 'MA', 100000.45, '2000-11-12T00:00:00'::timestamp, 99);
+
+query C rowsort
+select * from person;
+----
+1 jane smith 20 MA 100000.45 2000-11-12T00:00:00.000000000 99
+
+# TODO: support error instead of panicking
+# thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value:
SQL(ParserError("Expected AS, found: SELECT"))',
datafusion/core/tests/sqllogictests/src/main.rs:197:42
Review Comment:
I think I fixed this in https://github.com/apache/arrow-datafusion/pull/4530
-- so I think if you merge this PR up to master again it will work
--
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]
