qstommyshu commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2027702690
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -4665,18 +4674,18 @@ Projection: person.id, person.age
}
#[test]
-fn test_prepare_statement_infer_types_from_between_predicate() {
+fn test_infer_types_from_between_predicate() {
let sql = "SELECT id, age FROM person WHERE age BETWEEN $1 AND $2";
- let expected_plan = r#"
-Projection: person.id, person.age
- Filter: person.age BETWEEN $1 AND $2
- TableScan: person
- "#
- .trim();
-
- let expected_dt = "[Int32]";
Review Comment:
The function `fn generate_prepare_stmt_and_data_types(sql: &str) ->
(LogicalPlan, String)` already returns what we need — both the plan and the
inferred logical data types. However, the test named
`test_prepare_statement_infer_types_from_between_predicate()` is a bit
misleading, as it suggests we're testing a PREPARE statement and its associated
data types, but it is not.
In the original `prepare_stmt_quick_test()`, the `expected_dt` argument
isn't actually used in this specific test case — because the SQL being tested
is not a PREPARE statement. It doesn’t generate a `Statement::Prepare` variant
in the logical plan, so it never enters the matching arm in
`prepare_stmt_quick_test()`:
```Rust
// verify data types
if let LogicalPlan::Statement(Statement::Prepare(Prepare { data_types, ..
})) = assert_plan {
let dt = format!("{data_types:?}");
assert_eq!(dt, expected_data_types);
}
```
This is exactly why I chose to `panic!` in
`generate_prepare_stmt_and_data_types()` — to ensure we're only using it with
valid PREPARE statements. The original test was essentially relying on a
semantic bug, and what I did was make that behaviour explicit and correct.
I've updated other similar cases as well.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
