NGA-TRAN commented on code in PR #4561:
URL: https://github.com/apache/arrow-datafusion/pull/4561#discussion_r1044039751


##########
datafusion/core/tests/sql/select.rs:
##########
@@ -1257,6 +1258,73 @@ async fn csv_join_unaliased_subqueries() -> Result<()> {
     Ok(())
 }
 
+// Test prepare statement from sql to final result
+// This test is equivalent with the test parallel_query_with_filter below but 
using prepare statement
+#[tokio::test]
+async fn test_prepare_statement() -> Result<()> {

Review Comment:
   I want to have this test to verify my logical plan works correctly



##########
datafusion/sql/src/planner.rs:
##########
@@ -6311,9 +6308,18 @@ mod tests {
 
         let expected_dt = "[Int32]";
 
-        prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+        let plan = prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+
+        ///////////////////
+        // replace params with values
+        let param_values = vec![ScalarValue::Int32(Some(10))];
+        let expected_plan = "Projection: person.id, person.age\
+        \n  Filter: person.age = Int64(10)\
+        \n    TableScan: person";

Review Comment:
   All the available tests for prepare logical plan are used to replace params 
with values



##########
datafusion/sql/src/planner.rs:
##########
@@ -6324,7 +6330,54 @@ mod tests {
 
         let expected_dt = "[]";
 
-        prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+        let plan = prepare_stmt_quick_test(sql, expected_plan, expected_dt);
+
+        ///////////////////
+        // replace params with values
+        let param_values = vec![];
+        let expected_plan = "Projection: person.id, person.age\
+        \n  Filter: person.age = Int64(10)\
+        \n    TableScan: person";
+
+        prepare_stmt_replace_params_quick_test(plan, param_values, 
expected_plan);
+    }
+
+    #[test]
+    #[should_panic(expected = "value: Internal(\"Expected 1 parameters, got 
0\")")]
+    fn test_prepare_statement_to_plan_one_param_no_value_panic() {

Review Comment:
   And more panics are added for new functions



##########
datafusion/sql/src/planner.rs:
##########
@@ -6239,11 +6256,7 @@ mod tests {
         // 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);
+        logical_plan(sql).unwrap();

Review Comment:
   Instead of calling `prepare_stmt_quick_test` that will first generate 
logical plan (and then panic for this test), I call `logical_plan` directly to 
simplify the test but still test the same things



##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -534,6 +574,655 @@ impl LogicalPlan {
             _ => {}
         }
     }
+
+    /// recursively to replace the params (e.g $1 $2, ...) wit corresponding 
values provided in the prams_values
+    pub fn replace_params_with_values(
+        &self,
+        param_values: &Vec<ScalarValue>,
+    ) -> Result<LogicalPlan, DataFusionError> {

Review Comment:
   This is actually create a new logical plan. I hope sometime in the future 
someone will optimize this to only create expressions for placeholders/params 
and keep the rest the same. Not sure if that is possible/easy though



##########
datafusion/core/tests/sql/select.rs:
##########
@@ -1257,6 +1258,73 @@ async fn csv_join_unaliased_subqueries() -> Result<()> {
     Ok(())
 }
 
+// Test prepare statement from sql to final result
+// This test is equivalent with the test parallel_query_with_filter below but 
using prepare statement
+#[tokio::test]
+async fn test_prepare_statement() -> Result<()> {
+    let tmp_dir = TempDir::new()?;
+    let partition_count = 4;
+    let ctx = partitioned_csv::create_ctx(&tmp_dir, partition_count).await?;
+
+    // sql to statement then to prepare logical plan with parameters
+    // c1 defined as UINT32, c2 defined as UInt64 but the params are Int32 and 
Float64
+    let logical_plan =
+        ctx.create_logical_plan("PREPARE my_plan(INT, DOUBLE) AS SELECT c1, c2 
FROM test WHERE c1 > $2 AND c1 < $1")?;
+
+    // prepare logical plan to logical plan without parameters
+    let param_values = vec![ScalarValue::Int32(Some(3)), 
ScalarValue::Float64(Some(0.0))];
+    let logical_plan = LogicalPlan::execute(logical_plan, param_values)?;

Review Comment:
   This is the key test here to explicitly convert prepare logical plan to 
normal logical plan



-- 
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]

Reply via email to