qstommyshu commented on code in PR #15743:
URL: https://github.com/apache/datafusion/pull/15743#discussion_r2083691379


##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -4607,82 +4607,58 @@ fn test_prepare_statement_to_plan_params_as_constants() 
{
 }
 
 #[test]
-fn test_infer_types_from_join() {
+fn test_prepare_statement_infer_types_from_join() {
     let sql =
-        "SELECT id, order_id FROM person JOIN orders ON id = customer_id and 
age = $1";
+        "PREPARE my_plan AS SELECT id, order_id FROM person JOIN orders ON id 
= customer_id and age = $1";
 
     let plan = logical_plan(sql).unwrap();
     assert_snapshot!(
         plan,
         @r#"
-    Projection: person.id, orders.order_id
-      Inner Join:  Filter: person.id = orders.customer_id AND person.age = $1
-        TableScan: person
-        TableScan: orders
+    Prepare: "my_plan" [] 
+      Projection: person.id, orders.order_id
+        Inner Join:  Filter: person.id = orders.customer_id AND person.age = $1
+          TableScan: person
+          TableScan: orders
     "#
     );
 
     let actual_types = plan.get_parameter_types().unwrap();
     let expected_types = HashMap::from([("$1".to_string(), 
Some(DataType::Int32))]);
     assert_eq!(actual_types, expected_types);
-
-    // replace params with values
-    let param_values = vec![ScalarValue::Int32(Some(10))];
-    let plan_with_params = plan.with_param_values(param_values).unwrap();
-
-    assert_snapshot!(
-        plan_with_params,
-        @r"
-    Projection: person.id, orders.order_id
-      Inner Join:  Filter: person.id = orders.customer_id AND person.age = 
Int32(10)
-        TableScan: person
-        TableScan: orders
-    "
-    );

Review Comment:
   Got it, I see what you mean. This test can only be corrected once you set up 
the infer type feature. But the development flow would be weird if both this 
two PR addresses the same issue. 
   
   I think it would be better if we move everything to your new PR, so the new 
PR would include include `infer types` along with fixing/adding corresponding 
tests, and we only need to deal with one PR. We can also merge this PR first 
then move on to #16019 if @alamb @kczimm prefers this way.



-- 
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...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to