blaginin commented on code in PR #16324:
URL: https://github.com/apache/datafusion/pull/16324#discussion_r2136447543
##########
datafusion/core/tests/optimizer/mod.rs:
##########
@@ -107,16 +126,15 @@ fn concat_ws_literals() -> Result<()> {
let sql = "SELECT concat_ws('-', true, col_int32, false, null, 'hello',
col_utf8, 12, '', 3.4) \
AS col
FROM test";
- let expected =
- "Projection: concat_ws(Utf8(\"-\"), Utf8(\"true\"),
CAST(test.col_int32 AS Utf8), Utf8(\"false-hello\"), test.col_utf8,
Utf8(\"12--3.4\")) AS col\
- \n TableScan: test projection=[col_int32, col_utf8]";
- quick_test(sql, expected);
- Ok(())
-}
-
-fn quick_test(sql: &str, expected_plan: &str) {
let plan = test_sql(sql).unwrap();
- assert_eq!(expected_plan, format!("{plan}"));
+ assert_snapshot!(
+ format!("{plan}"),
Review Comment:
assert_snapshot triggers format internally so you can just do
```suggestion
plan,
```
##########
datafusion/core/tests/sql/explain_analyze.rs:
##########
@@ -52,43 +54,45 @@ async fn explain_analyze_baseline_metrics() {
let formatted = arrow::util::pretty::pretty_format_batches(&results)
.unwrap()
.to_string();
+
println!("Query Output:\n\n{formatted}");
- assert_metrics!(
- &formatted,
- "AggregateExec: mode=Partial, gby=[]",
- "metrics=[output_rows=3, elapsed_compute="
- );
- assert_metrics!(
- &formatted,
- "AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1]",
- "metrics=[output_rows=5, elapsed_compute="
- );
- assert_metrics!(
- &formatted,
- "FilterExec: c13@1 != C2GT5KVyOPZpgKVl110TyZO0NcJ434",
- "metrics=[output_rows=99, elapsed_compute="
- );
- assert_metrics!(
- &formatted,
- "ProjectionExec: expr=[]",
- "metrics=[output_rows=5, elapsed_compute="
- );
- assert_metrics!(
- &formatted,
- "CoalesceBatchesExec: target_batch_size=4096",
- "metrics=[output_rows=5, elapsed_compute"
- );
- assert_metrics!(
- &formatted,
- "UnionExec",
- "metrics=[output_rows=3, elapsed_compute="
- );
- assert_metrics!(
- &formatted,
- "WindowAggExec",
- "metrics=[output_rows=1, elapsed_compute="
- );
+ let re = Regex::new(r"\|[^|]*\|([^|]*)\|").unwrap();
+ let actual = formatted
+ .lines()
+ .map(|line| re.replace_all(line, "$1").trim_end().to_string())
+ .filter(|line| !line.is_empty() && !line.starts_with('+'))
+ .collect::<Vec<_>>()
+ .join("\n");
Review Comment:
what do you think about just asserting the table itself without
modifications? I fear that this code will:
a - make test a bit more complicated
b - needs to be manually maintained when we change the formatting
##########
datafusion/core/tests/sql/explain_analyze.rs:
##########
@@ -52,43 +54,45 @@ async fn explain_analyze_baseline_metrics() {
let formatted = arrow::util::pretty::pretty_format_batches(&results)
.unwrap()
.to_string();
+
println!("Query Output:\n\n{formatted}");
- assert_metrics!(
- &formatted,
- "AggregateExec: mode=Partial, gby=[]",
- "metrics=[output_rows=3, elapsed_compute="
- );
- assert_metrics!(
- &formatted,
- "AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1]",
- "metrics=[output_rows=5, elapsed_compute="
- );
- assert_metrics!(
- &formatted,
- "FilterExec: c13@1 != C2GT5KVyOPZpgKVl110TyZO0NcJ434",
- "metrics=[output_rows=99, elapsed_compute="
- );
- assert_metrics!(
- &formatted,
- "ProjectionExec: expr=[]",
- "metrics=[output_rows=5, elapsed_compute="
- );
- assert_metrics!(
- &formatted,
- "CoalesceBatchesExec: target_batch_size=4096",
- "metrics=[output_rows=5, elapsed_compute"
- );
- assert_metrics!(
- &formatted,
- "UnionExec",
- "metrics=[output_rows=3, elapsed_compute="
- );
- assert_metrics!(
- &formatted,
- "WindowAggExec",
- "metrics=[output_rows=1, elapsed_compute="
- );
+ let re = Regex::new(r"\|[^|]*\|([^|]*)\|").unwrap();
+ let actual = formatted
+ .lines()
+ .map(|line| re.replace_all(line, "$1").trim_end().to_string())
+ .filter(|line| !line.is_empty() && !line.starts_with('+'))
+ .collect::<Vec<_>>()
+ .join("\n");
+ insta::with_settings!({filters => vec![
+ (r"\d+\.?\d*[µmn]?s", "[TIME]"),
Review Comment:
`[µmn]` - I think this can be somewhat flaky it the test takes more time -
maybe match on `[]`?
##########
datafusion/core/tests/optimizer/mod.rs:
##########
@@ -107,16 +126,15 @@ fn concat_ws_literals() -> Result<()> {
let sql = "SELECT concat_ws('-', true, col_int32, false, null, 'hello',
col_utf8, 12, '', 3.4) \
AS col
FROM test";
- let expected =
- "Projection: concat_ws(Utf8(\"-\"), Utf8(\"true\"),
CAST(test.col_int32 AS Utf8), Utf8(\"false-hello\"), test.col_utf8,
Utf8(\"12--3.4\")) AS col\
- \n TableScan: test projection=[col_int32, col_utf8]";
- quick_test(sql, expected);
- Ok(())
-}
-
-fn quick_test(sql: &str, expected_plan: &str) {
let plan = test_sql(sql).unwrap();
- assert_eq!(expected_plan, format!("{plan}"));
+ assert_snapshot!(
+ format!("{plan}"),
Review Comment:
same above
##########
datafusion/core/tests/sql/explain_analyze.rs:
##########
@@ -797,14 +796,16 @@ async fn explain_physical_plan_only() {
let sql = "EXPLAIN select count(*) from (values ('a', 1, 100), ('a', 2,
150)) as t (c1,c2,c3)";
let actual = execute(&ctx, sql).await;
let actual = normalize_vec_for_explain(actual);
Review Comment:
<img width="760" alt="image"
src="https://github.com/user-attachments/assets/e16521c6-82f3-49dd-83ad-38237f2c16f1";
/>
feels like these can also be insta redactions?
--
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
