kczimm commented on code in PR #15743:
URL: https://github.com/apache/datafusion/pull/15743#discussion_r2080478955
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -4695,34 +4698,37 @@ fn test_infer_types_from_between_predicate() {
// replace params with values
let param_values = vec![ScalarValue::Int32(Some(10)),
ScalarValue::Int32(Some(30))];
- let plan_with_params = plan.with_param_values(param_values).unwrap();
-
- assert_snapshot!(
- plan_with_params,
- @r"
- Projection: person.id, person.age
- Filter: person.age BETWEEN Int32(10) AND Int32(30)
- TableScan: person
- "
- );
+ let plan_with_params = plan.with_param_values(param_values);
+ match plan_with_params {
+ Ok(plan_p) => assert_snapshot!(
+ plan_p,
+ @r"
+ Projection: person.id, person.age
+ Filter: person.age BETWEEN Int32(10) AND Int32(30)
+ TableScan: person
+ "
+ ),
+ Err(err) => print!("error: {:?}", err),
Review Comment:
If `LogicalPlan::with_param_values` fails, we want this test to fail, right?
##########
datafusion/sql/src/statement.rs:
##########
@@ -710,6 +710,25 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
*statement,
&mut planner_context,
)?;
+
+ //Get inferred data_types from the plan if it is empty in the
prepare statement
+ let data_types = match data_types.is_empty() {
+ true => {
+ let mut data_types: Vec<DataType> = plan
+ .get_parameter_types()?
+ .iter()
+ .filter_map(|d| match d {
+ (_, Some(v)) => Some(v.clone()),
+ _ => None,
+ })
+ .collect::<Vec<DataType>>();
+ data_types.sort();
+
planner_context.with_prepare_param_data_types(data_types.clone());
+ data_types
+ }
+ false => data_types,
+ };
+
Review Comment:
`LogicalPlan` variants other than `Prepare` can have `Expr::Placeholder`
present, though.
##########
datafusion/sql/src/statement.rs:
##########
@@ -710,6 +710,25 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
*statement,
&mut planner_context,
)?;
+
+ //Get inferred data_types from the plan if it is empty in the
prepare statement
+ let data_types = match data_types.is_empty() {
+ true => {
+ let mut data_types: Vec<DataType> = plan
+ .get_parameter_types()?
+ .iter()
+ .filter_map(|d| match d {
+ (_, Some(v)) => Some(v.clone()),
+ _ => None,
+ })
+ .collect::<Vec<DataType>>();
+ data_types.sort();
+
planner_context.with_prepare_param_data_types(data_types.clone());
+ data_types
+ }
+ false => data_types,
+ };
Review Comment:
If you make `data_types` mutable above, you can do something like this which
is a bit more terse:
```suggestion
if data_types.is_empty() {
for dt in
plan.get_parameter_types()?.into_values().into_iter() {
if let Some(dt) = dt {
data_types.push(dt);
}
}
data_types.sort();
planner_context.with_prepare_param_data_types(data_types.clone());
}
```
--
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
