You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "qstommyshu (via GitHub)" <gi...@apache.org> on 2025/04/04 14:08:38 UTC
[PR] Migrate datafusion/sql tests to insta, part6 [datafusion]
qstommyshu opened a new pull request, #15578:
URL: https://github.com/apache/datafusion/pull/15578
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Related #15484, #15397, #15497, #15499, #15533, #15548 , #15567
- 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
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes 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, Part5: #15567
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes, I manually tested the before/after changes.
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate datafusion/sql tests to insta, part6 [datafusion]
Posted by "qstommyshu (via GitHub)" <gi...@apache.org>.
qstommyshu commented on PR #15578:
URL: https://github.com/apache/datafusion/pull/15578#issuecomment-2781439769
> Hi @alamb and @blaginin
>
> I found a way to migrate `roundtrip_statement_with_dialect()` now, just want to confirm if you like to proceed with it.
>
> What I did below is to incorporate all core test logics into a macro, and try to minimize the lines of code (so that we don't have too much lines of code increase in this migration) for each test case but still expressive, by adding the variable name before each variable in the macro (because rust analyzer doesn't indicate types for macro).
>
> ```rust
> #[macro_export]
> macro_rules! roundtrip_statement_with_dialect_helper {
> (
> query: $sql:expr,
> parser_dialect: $parser_dialect:expr,
> unparser_dialect: $unparser_dialect:expr,
> expected: @ $expected:literal $(,)?
> ) => {{
> let statement = Parser::new(&$parser_dialect)
> .try_with_sql($sql)?
> .parse_statement()?;
>
> let state = MockSessionState::default()
> .with_aggregate_function(max_udaf())
> .with_aggregate_function(min_udaf())
> .with_expr_planner(Arc::new(CoreFunctionPlanner::default()))
> .with_expr_planner(Arc::new(NestedFunctionPlanner));
>
> let context = MockContextProvider { state };
> let sql_to_rel = SqlToRel::new(&context);
> let plan = sql_to_rel
> .sql_statement_to_plan(statement)
> .unwrap_or_else(|e| panic!("Failed to parse sql: {}\n{e}", $sql));
>
> let unparser = Unparser::new(&$unparser_dialect);
> let roundtrip_statement = unparser.plan_to_sql(&plan)?;
>
> let actual = &roundtrip_statement.to_string();
> insta::assert_snapshot!(actual, @ $expected);
> }};
> }
> ```
>
> ```rust
> // each test case would look like this
> #[test]
> fn roundtrip_statement_with_dialect_1() -> Result<(), DataFusionError> {
> roundtrip_statement_with_dialect_helper!(
> query: "select min(ta.j1_id) as j1_min from j1 ta order by min(ta.j1_id) limit 10;",
> parser_dialect: MySqlDialect {},
> unparser_dialect: UnparserMySqlDialect {},
> expected: @"SELECT `j1_min` FROM (SELECT min(`ta`.`j1_id`) AS `j1_min`, min(`ta`.`j1_id`) FROM `j1` AS `ta` ORDER BY min(`ta`.`j1_id`) ASC) AS `derived_sort` LIMIT 10",
> );
> Ok(())
> }
> ```
>
> Note: we can't put this macro in a for loop because inline `assert_snapshot!($actual, @ $expected)` only takes literal as expected argument, so we can't store the expected value in a variable and then pass to the macro (expr != literal).
>
> And I feel like this is not a standard way to write test, not sure if I should go head and do it or not. Please let me know if you want to proceed with this way.
Just want to confirm, do I need to migrate the big test case `roundtrip_statement_with_dialect() `?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate datafusion/sql tests to insta, part6 [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #15578:
URL: https://github.com/apache/datafusion/pull/15578#discussion_r2030152111
##########
datafusion/sql/tests/cases/diagnostic.rs:
##########
@@ -136,7 +137,7 @@ fn test_table_not_found() -> Result<()> {
let query = "SELECT * FROM /*a*/personx/*a*/";
let spans = get_spans(query);
let diag = do_query(query);
- assert_eq!(diag.message, "table 'personx' not found");
+ assert_snapshot!(diag.message, @"table 'personx' not found");
Review Comment:
This is fine though single string comparisons are probably fine to just leave as assert_eq in the future
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -62,46 +63,44 @@ use sqlparser::dialect::{Dialect, GenericDialect, MySqlDialect};
use sqlparser::parser::Parser;
#[test]
-fn roundtrip_expr() {
- let tests: Vec<(TableReference, &str, &str)> = vec![
- (TableReference::bare("person"), "age > 35", r#"(age > 35)"#),
- (
- TableReference::bare("person"),
- "id = '10'",
- r#"(id = '10')"#,
- ),
- (
- TableReference::bare("person"),
- "CAST(id AS VARCHAR)",
- r#"CAST(id AS VARCHAR)"#,
- ),
- (
- TableReference::bare("person"),
- "sum((age * 2))",
- r#"sum((age * 2))"#,
- ),
- ];
+fn test_roundtrip_expr_1() {
Review Comment:
this looks good
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -1284,10 +1446,10 @@ fn test_join_with_table_scan_filters() -> Result<()> {
.build()?;
let sql = plan_to_sql(&join_plan_with_filter)?;
Review Comment:
this is realyl nice
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -726,121 +722,276 @@ fn test_aggregation_without_projection() -> Result<()> {
let unparser = Unparser::default();
let statement = unparser.plan_to_sql(&plan)?;
-
- let actual = &statement.to_string();
-
- assert_eq!(
- actual,
- r#"SELECT sum(users.age), users."name" FROM users GROUP BY users."name""#
+ assert_snapshot!(
+ statement,
+ @r#"SELECT sum(users.age), users."name" FROM users GROUP BY users."name""#
);
Ok(())
}
#[test]
-fn test_table_references_in_plan_to_sql() {
- fn test(table_name: &str, expected_sql: &str, dialect: &impl UnparserDialect) {
- let schema = Schema::new(vec![
- Field::new("id", DataType::Utf8, false),
- Field::new("value", DataType::Utf8, false),
- ]);
- let plan = table_scan(Some(table_name), &schema, None)
- .unwrap()
- .project(vec![col("id"), col("value")])
- .unwrap()
- .build()
- .unwrap();
-
- let unparser = Unparser::new(dialect);
- let sql = unparser.plan_to_sql(&plan).unwrap();
-
- assert_eq!(sql.to_string(), expected_sql)
- }
-
- test(
- "catalog.schema.table",
- r#"SELECT "table".id, "table"."value" FROM "catalog"."schema"."table""#,
+fn test_table_references_in_plan_to_sql_1() {
+ let table_name = "catalog.schema.table";
Review Comment:
would it be possible to reduce the replication here of recreating the schema so many times?
Something like
```rust
/// return a schema with two string columns: "id" and "value"
fn test_schema() -> Schema {
Schema::new(vec![
Field::new("id", DataType::Utf8, false),
Field::new("value", DataType::Utf8, false),
]);
}
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate datafusion/sql tests to insta, part6 [datafusion]
Posted by "qstommyshu (via GitHub)" <gi...@apache.org>.
qstommyshu commented on PR #15578:
URL: https://github.com/apache/datafusion/pull/15578#issuecomment-2779584934
Also, I found the test cases are a bit here and there, but I think the sql test suite are too large to restructure now. I'm thinking maybe we can try to use modules to separate different tests in future?
Here is an example:
```Rust
pub fn add(a: i32, b: i32) -> i32 {
a + b
}
pub fn multiply(a: i32, b: i32) -> i32 {
a * b
}
#[cfg(test)]
mod tests {
use super::*;
// add test module
mod add_tests {
use super::*;
#[test]
fn test_add_positive() {
assert_eq!(add(2, 3), 5);
}
#[test]
fn test_add_negative() {
assert_eq!(add(-2, -3), -5);
}
}
// multiply test module
mod multiply_tests {
use super::*;
#[test]
fn test_multiply_positive() {
assert_eq!(multiply(4, 5), 20);
}
#[test]
fn test_multiply_mixed() {
assert_eq!(multiply(-3, 6), -18);
}
}
// integration test module
mod integration_tests {
use super::*;
#[test]
fn test_combined_operations() {
let sum = add(2, 3);
let product = multiply(sum, 4);
assert_eq!(product, 20);
}
}
}
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate datafusion/sql tests to insta, part6 [datafusion]
Posted by "qstommyshu (via GitHub)" <gi...@apache.org>.
qstommyshu commented on PR #15578:
URL: https://github.com/apache/datafusion/pull/15578#issuecomment-2779568810
This is the last PR for #15397
The migration is mostly done. There are just a few really big tests that I think they would be too big to break down and use inline assertion with `assert_snapshot!` (For example: `roundtrip_statement()`, `roundtrip_statement_with_dialect()`).
Please let me know if I need to break them down.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate datafusion/sql tests to insta, part6 [datafusion]
Posted by "qstommyshu (via GitHub)" <gi...@apache.org>.
qstommyshu commented on PR #15578:
URL: https://github.com/apache/datafusion/pull/15578#issuecomment-2781841733
Got it, Thanks @alamb for your thoughts.
I will update `roundtrip_statement_with_dialect()`. I will probably go with the macro approach because the macro approach is essentially the same as the original test, so I can almost follow the original `TestStatementWithDialect` structure. Plus, I don't need to create a new struct and impl traits.
I can foresee the code changes will be large, so I will leave it to the next PR (part 7, should be the *real* last part). Please merge this PR if you think the change is good.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate datafusion/sql tests to insta, part6 [datafusion]
Posted by "qstommyshu (via GitHub)" <gi...@apache.org>.
qstommyshu commented on PR #15578:
URL: https://github.com/apache/datafusion/pull/15578#issuecomment-2781397695
Hi @alamb and @blaginin
I found a way to migrate `roundtrip_statement_with_dialect()` now, just want to confirm if you like to proceed with it. What I did below is to incorporate all core functions into a macro, and try to minimize the lines of code (so that we don't have too much lines of code increase in this migration) for each test case but still expressive, by adding the variable name before each variable in the macro (because rust analyzer doesn't indicate types for macro).
```Rust
#[macro_export]
macro_rules! roundtrip_statement_with_dialect_helper {
(
query: $sql:expr,
parser_dialect: $parser_dialect:expr,
unparser_dialect: $unparser_dialect:expr,
expected: @ $expected:literal $(,)?
) => {{
let statement = Parser::new(&$parser_dialect)
.try_with_sql($sql)?
.parse_statement()?;
let state = MockSessionState::default()
.with_aggregate_function(max_udaf())
.with_aggregate_function(min_udaf())
.with_expr_planner(Arc::new(CoreFunctionPlanner::default()))
.with_expr_planner(Arc::new(NestedFunctionPlanner));
let context = MockContextProvider { state };
let sql_to_rel = SqlToRel::new(&context);
let plan = sql_to_rel
.sql_statement_to_plan(statement)
.unwrap_or_else(|e| panic!("Failed to parse sql: {}\n{e}", $sql));
let unparser = Unparser::new(&$unparser_dialect);
let roundtrip_statement = unparser.plan_to_sql(&plan)?;
let actual = &roundtrip_statement.to_string();
insta::assert_snapshot!(actual, @ $expected);
}};
}
// each test case would look like this
#[test]
fn roundtrip_statement_with_dialect_1() -> Result<(), DataFusionError> {
roundtrip_statement_with_dialect_helper!(
query: "select min(ta.j1_id) as j1_min from j1 ta order by min(ta.j1_id) limit 10;",
parser_dialect: MySqlDialect {},
unparser_dialect: UnparserMySqlDialect {},
expected: @"SELECT `j1_min` FROM (SELECT min(`ta`.`j1_id`) AS `j1_min`, min(`ta`.`j1_id`) FROM `j1` AS `ta` ORDER BY min(`ta`.`j1_id`) ASC) AS `derived_sort` LIMIT 10",
);
Ok(())
}
```
However, I feel like this is not a standard way to write test. Please let me know if you want to proceed with 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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate datafusion/sql tests to insta, part6 [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15578:
URL: https://github.com/apache/datafusion/pull/15578#issuecomment-2781575831
> Just want to confirm, do I need to migrate the big test case roundtrip_statement_with_dialect() ?
IN my opinion, it is is note required. If you want to rewrite it I am sure it would be appreciated by future contributors
Another pattern is to put the entire test into a structure that has a `Display` impl and then print out that display impl in the snapshot. I did something like this in https://github.com/pydantic/datafusion/pull/16
So instead of
```rust
// each test case would look like this
#[test]
fn roundtrip_statement_with_dialect_1() -> Result<(), DataFusionError> {
roundtrip_statement_with_dialect_helper!(
query: "select min(ta.j1_id) as j1_min from j1 ta order by min(ta.j1_id) limit 10;",
parser_dialect: MySqlDialect {},
unparser_dialect: UnparserMySqlDialect {},
expected: @"SELECT `j1_min` FROM (SELECT min(`ta`.`j1_id`) AS `j1_min`, min(`ta`.`j1_id`) FROM `j1` AS `ta` ORDER BY min(`ta`.`j1_id`) ASC) AS `derived_sort` LIMIT 10",
);
Ok(())
}
```
```rust
struct TestResult {
input_query: String,
parser_dialect: &Dialect,
unparser_dialect: &Dialect,
output_query: String
}
impl TestResult {
fn try_new(
input_query: String,
parser_dialect: &Dialect,
unparser_dialect: &Dialect,
) -> Result<Self> {
// do unparse , etc
}
}
impl Display for TestResult { ... }
// tests make a TestResult and compare the string result
#[test]
fn roundtrip_statement_with_dialect_1() -> Result<(), DataFusionError> {
let result =
insta::assert_snapshot!(
TestResult::try_new(
select min(ta.j1_id) as j1_min from j1 ta order by min(ta.j1_id) limit 10;",
MySqlDialect {},
UnparserMySqlDialect {},
},
@r"
query: "select min(ta.j1_id) as j1_min from j1 ta order by min(ta.j1_id) limit 10;",
parser_dialect: MySqlDialect {},
unparser_dialect: UnparserMySqlDialect {},
output "SELECT `j1_min` FROM (SELECT min(`ta`.`j1_id`) AS `j1_min`, min(`ta`.`j1_id`) FROM `j1` AS `ta` ORDER BY min(`ta`.`j1_id`) ASC) AS `derived_sort` LIMIT 10",
);
}
}
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate datafusion/sql tests to insta, part6 [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15578:
URL: https://github.com/apache/datafusion/pull/15578#issuecomment-2782936412
Thanks @qstommyshu !
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate datafusion/sql tests to insta, part6 [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #15578:
URL: https://github.com/apache/datafusion/pull/15578
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org