[PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
qstommyshu opened a new pull request, #15567: URL: https://github.com/apache/datafusion/pull/15567 ## Which issue does this PR close? - Related #15484, #15397, #15497, #15499, #15533, #15548 - this is a part of #15484 breaking down. - Checkout things to note of the whole migration in comments section of #15484. ## Rationale for this change ## What changes are included in this PR? This is the part 5 of #15484 breakdown, as the code changes in #15484 is too large. Part1: #15497, Part2: #15499, Part3: #15533, Part4: 15548 ## Are these changes tested? Yes, I manually tested the before/after changes. ## Are there any user-facing changes? No -- 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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
blaginin commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2027650462
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -3388,26 +3389,15 @@ fn
ident_normalization_parser_options_ident_normalization() -> ParserOptions {
}
}
-fn prepare_stmt_quick_test(
-sql: &str,
-expected_plan: &str,
-expected_data_types: &str,
-) -> LogicalPlan {
+fn generate_prepare_stmt_and_data_types(sql: &str) -> (LogicalPlan, String) {
Review Comment:
to make output arguments consistent, should you return `data_types` directly
and compare them via `assert_debug_snapshot`?
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
qstommyshu commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2027712552
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -3388,26 +3389,15 @@ fn
ident_normalization_parser_options_ident_normalization() -> ParserOptions {
}
}
-fn prepare_stmt_quick_test(
-sql: &str,
-expected_plan: &str,
-expected_data_types: &str,
-) -> LogicalPlan {
+fn generate_prepare_stmt_and_data_types(sql: &str) -> (LogicalPlan, String) {
Review Comment:
Just looked into this method 👀. `assert_debug_snapshot` does not work with
inline snapshot. We would have to store the snapshot in the `snapshot/` folder
(which I suppose that's not what we want?).
In terms of the output arguments, I probably need to use an `if let` or
another function to check the data type if we want to return `LogicalPlan`.
Then the testing procedure for a prepare statement would be:
```Rust
let plan = logical_plan(sql).unwrap();
if let LogicalPlan::Statement(Statement::Prepare(Prepare { data_types, ..
})) = plan {
assert_snapshot!(data_types, @"SOME DATA TYPE");
}
assert_snapshot!(
plan,
@r#"
Prepare: "my_plan" [Int32]
Projection: person.id, person.age
Filter: person.age = Int64(10)
TableScan: person
"#
);
```
instead of what we have now:
```Rust
let (plan, dt) = generate_prepare_stmt_and_data_types(sql);
assert_snapshot!(
plan,
@r#"
Prepare: "my_plan" [Int32]
Projection: person.id, person.age
Filter: person.age = Int64(10)
TableScan: person
"#
);
assert_snapshot!(dt, @r#"[Int32]"#);
```
I can change it if you prefer the first testing procedure.
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
alamb commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2028787617
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -3388,26 +3389,15 @@ fn
ident_normalization_parser_options_ident_normalization() -> ParserOptions {
}
}
-fn prepare_stmt_quick_test(
-sql: &str,
-expected_plan: &str,
-expected_data_types: &str,
-) -> LogicalPlan {
+fn generate_prepare_stmt_and_data_types(sql: &str) -> (LogicalPlan, String) {
Review Comment:
I like what you have here 👍
##
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:
This makes sense -- thank you
I actually looks to me like it is a mistake in the test 🤔
Perhaps the test should be something like
```rust
let sql = "PREPARE SELECT id, age FROM person WHERE age BETWEEN $1 AND
$2";
```
Instead of
```rust
let sql = "SELECT id, age FROM person WHERE age BETWEEN $1 AND $2";
```
(maybe we can fix this in a follow on PR -- I will file a ticket)
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
alamb merged PR #15567: URL: https://github.com/apache/datafusion/pull/15567 -- 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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
alamb commented on PR #15567: URL: https://github.com/apache/datafusion/pull/15567#issuecomment-2778708271 Let's keep the project and momentum -- merging this one in! -- 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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
alamb commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2028798298
##
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:
- Filed https://github.com/apache/datafusion/issues/15577 to track
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
qstommyshu commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2027712552
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -3388,26 +3389,15 @@ fn
ident_normalization_parser_options_ident_normalization() -> ParserOptions {
}
}
-fn prepare_stmt_quick_test(
-sql: &str,
-expected_plan: &str,
-expected_data_types: &str,
-) -> LogicalPlan {
+fn generate_prepare_stmt_and_data_types(sql: &str) -> (LogicalPlan, String) {
Review Comment:
Just looked into this method 👀. `assert_debug_snapshot` does not work with
inline snapshot. We would have to store the snapshot in the `snapshot/` folder
(which I suppose that's not what we want?).
In terms of the output arguments, I probably need to create another to
generate the data type if we want to return `LogicalPlan`.
Then the testing procedure for a prepare statement would be:
```Rust
let plan = logical_plan(sql).unwrap();
if let LogicalPlan::Statement(Statement::Prepare(Prepare { data_types, ..
})) = plan {
assert_snapshot!(data_types, @"SOME DATA TYPE");
}
assert_snapshot!(
plan,
@r#"
Prepare: "my_plan" [Int32]
Projection: person.id, person.age
Filter: person.age = Int64(10)
TableScan: person
"#
);
```
instead of what we have now:
```Rust
let (plan, dt) = generate_prepare_stmt_and_data_types(sql);
assert_snapshot!(
plan,
@r#"
Prepare: "my_plan" [Int32]
Projection: person.id, person.age
Filter: person.age = Int64(10)
TableScan: person
"#
);
assert_snapshot!(dt, @r#"[Int32]"#);
```
I can change it if you prefer the first testing procedure.
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
qstommyshu commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2027712552
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -3388,26 +3389,15 @@ fn
ident_normalization_parser_options_ident_normalization() -> ParserOptions {
}
}
-fn prepare_stmt_quick_test(
-sql: &str,
-expected_plan: &str,
-expected_data_types: &str,
-) -> LogicalPlan {
+fn generate_prepare_stmt_and_data_types(sql: &str) -> (LogicalPlan, String) {
Review Comment:
Just took a closer look at this method 👀. It turns out that
`assert_debug_snapshot!` doesn’t support inline snapshots — it always writes to
an external .snap file under the snapshots/ directory (which I assume we’re
trying to avoid?).
As for the output arguments — if we want to return just the `LogicalPlan`,
I’d need to extract the `data_types` separately using pattern matching or a
helper function, and then snapshot that field explicitly.
So the updated test flow for a `PREPARE` statement would look like this:
```Rust
let plan = logical_plan(sql).unwrap();
if let LogicalPlan::Statement(Statement::Prepare(Prepare { data_types, ..
})) = plan {
assert_snapshot!(data_types, @r"SOME DATA TYPE");
}
assert_snapshot!(
plan,
@r#"
Prepare: "my_plan" [Int32]
Projection: person.id, person.age
Filter: person.age = Int64(10)
TableScan: person
"#
);
```
instead of what we have now:
```Rust
let (plan, dt) = generate_prepare_stmt_and_data_types(sql);
assert_snapshot!(
plan,
@r#"
Prepare: "my_plan" [Int32]
Projection: person.id, person.age
Filter: person.age = Int64(10)
TableScan: person
"#
);
assert_snapshot!(dt, @r#"[Int32]"#);
```
I’m happy to switch to the first version if that’s preferred — just let me
know what works best for you! @blaginin @alamb
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
qstommyshu commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2027712552
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -3388,26 +3389,15 @@ fn
ident_normalization_parser_options_ident_normalization() -> ParserOptions {
}
}
-fn prepare_stmt_quick_test(
-sql: &str,
-expected_plan: &str,
-expected_data_types: &str,
-) -> LogicalPlan {
+fn generate_prepare_stmt_and_data_types(sql: &str) -> (LogicalPlan, String) {
Review Comment:
Just looked into this method 👀. `assert_debug_snapshot` does not work with
inline snapshot. We would have to store the snapshot in the `snapshot/` folder
(which I suppose that's not what we want?).
In terms of the output arguments, I probably need to use an `if let` or
another function to check the data type if we want to return `LogicalPlan`.
Then the testing procedure for a prepare statement would be:
```Rust
let plan = logical_plan(sql).unwrap();
if let LogicalPlan::Statement(Statement::Prepare(Prepare { data_types, ..
})) = plan {
assert_snapshot!(data_types, @r"SOME DATA TYPE");
}
assert_snapshot!(
plan,
@r#"
Prepare: "my_plan" [Int32]
Projection: person.id, person.age
Filter: person.age = Int64(10)
TableScan: person
"#
);
```
instead of what we have now:
```Rust
let (plan, dt) = generate_prepare_stmt_and_data_types(sql);
assert_snapshot!(
plan,
@r#"
Prepare: "my_plan" [Int32]
Projection: person.id, person.age
Filter: person.age = Int64(10)
TableScan: person
"#
);
assert_snapshot!(dt, @r#"[Int32]"#);
```
I can change it if you prefer the first testing procedure.
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
qstommyshu commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2027712552
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -3388,26 +3389,15 @@ fn
ident_normalization_parser_options_ident_normalization() -> ParserOptions {
}
}
-fn prepare_stmt_quick_test(
-sql: &str,
-expected_plan: &str,
-expected_data_types: &str,
-) -> LogicalPlan {
+fn generate_prepare_stmt_and_data_types(sql: &str) -> (LogicalPlan, String) {
Review Comment:
Just looked into this method 👀. `assert_debug_snapshot` does not work with
inline snapshot. We would have to store the snapshot in the `snapshot/` folder
(which I suppose that's not what we want?).
In terms of the output arguments, I probably need to use an `if let` to
check the data type if we want to return `LogicalPlan`.
Then the testing procedure for a prepare statement would be:
```Rust
let plan = logical_plan(sql).unwrap();
if let LogicalPlan::Statement(Statement::Prepare(Prepare { data_types, ..
})) = plan {
assert_snapshot!(data_types, @"SOME DATA TYPE");
}
assert_snapshot!(
plan,
@r#"
Prepare: "my_plan" [Int32]
Projection: person.id, person.age
Filter: person.age = Int64(10)
TableScan: person
"#
);
```
instead of what we have now:
```Rust
let (plan, dt) = generate_prepare_stmt_and_data_types(sql);
assert_snapshot!(
plan,
@r#"
Prepare: "my_plan" [Int32]
Projection: person.id, person.age
Filter: person.age = Int64(10)
TableScan: person
"#
);
assert_snapshot!(dt, @r#"[Int32]"#);
```
I can change it if you prefer the first testing procedure.
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
alamb commented on PR #15567: URL: https://github.com/apache/datafusion/pull/15567#issuecomment-2776729981 is this the last one? -- 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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
qstommyshu commented on PR #15567: URL: https://github.com/apache/datafusion/pull/15567#issuecomment-2776882350 > is this the last one? This is the last one for migrating tests in `tests/sql_integration.rs`. There are still some cases in `tests/cases/plan_to_sql.rs` and `tests/cases/diagnostic.rs`. I suppose this is the second last one (that's my guess) as the cases in those two files are much less and simpler. The last PR will be migrating tests in `tests/cases/plan_to_sql.rs` and `tests/cases/diagnostic.rs` to `insta`. -- 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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
qstommyshu commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2027712552
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -3388,26 +3389,15 @@ fn
ident_normalization_parser_options_ident_normalization() -> ParserOptions {
}
}
-fn prepare_stmt_quick_test(
-sql: &str,
-expected_plan: &str,
-expected_data_types: &str,
-) -> LogicalPlan {
+fn generate_prepare_stmt_and_data_types(sql: &str) -> (LogicalPlan, String) {
Review Comment:
Just looked into this method 👀. `assert_debug_snapshot` does not work with
inline snapshot. We would have to store the snapshot in the `snapshot/` folder
(which I suppose that's not what we want?).
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
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.
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
blaginin commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2027650462
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -3388,26 +3389,15 @@ fn
ident_normalization_parser_options_ident_normalization() -> ParserOptions {
}
}
-fn prepare_stmt_quick_test(
-sql: &str,
-expected_plan: &str,
-expected_data_types: &str,
-) -> LogicalPlan {
+fn generate_prepare_stmt_and_data_types(sql: &str) -> (LogicalPlan, String) {
Review Comment:
to make output arguments consistent, do you think you could you return
`data_types` directly and compare them via `assert_debug_snapshot`?
--
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]
Re: [PR] Migrate datafusion/sql tests to insta, part5 [datafusion]
alamb commented on code in PR #15567:
URL: https://github.com/apache/datafusion/pull/15567#discussion_r2027613057
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -15,6 +15,7 @@
// specific language governing permissions and limitations
// under the License.
+use core::panic;
Review Comment:
I haven't seen `panic` explicitly imported before (I think panic is in the
standard prelude) 🤔
##
datafusion/sql/tests/sql_integration.rs:
##
@@ -15,6 +15,7 @@
// specific language governing permissions and limitations
// under the License.
+use core::panic;
Review Comment:
```suggestion
```
##
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:
it seems like this lost the datatype somehow
Maybe we can make a function that returns the plan string as well as the
logical datatype?
--
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]
