blaginin commented on code in PR #16184:
URL: https://github.com/apache/datafusion/pull/16184#discussion_r2106160686
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -2759,10 +2749,24 @@ mod tests {
let join = LogicalPlanBuilder::from(left).cross_join(right)?.build()?;
- let _ = LogicalPlanBuilder::from(join.clone())
+ let plan = LogicalPlanBuilder::from(join.clone())
.union(join)?
.build()?;
+ assert_snapshot!(plan, @r"
+ Union
+ Cross Join:
+ SubqueryAlias: left
Review Comment:
that wasn't here before but i think adding it won't hurt
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -2592,19 +2576,25 @@ mod tests {
let err = nested_table_scan("test_table")?
.unnest_column("scalar")
.unwrap_err();
- assert!(err
- .to_string()
- .starts_with("Internal error: trying to unnest on invalid data
type UInt32"));
+
+ let err_msg = err
+ .strip_backtrace()
+ .lines()
+ .filter(|line| !line.contains("This was likely caused by a bug"))
Review Comment:
IMO, that defeats the purpose of the fixture since you'll still have to
manually update this line if the format changes
Maybe we can
[filters](https://docs.rs/insta/latest/insta/struct.Settings.html#method.add_filter)
for this case?
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4380,18 +4364,13 @@ digraph {
};
let plan = test_plan();
let res = plan.visit_with_subqueries(&mut visitor).unwrap_err();
- assert_eq!(
- "This feature is not implemented: Error in post_visit",
- res.strip_backtrace()
+ assert_snapshot!(
+ res.strip_backtrace(),
+ @"This feature is not implemented: Error in post_visit"
);
- assert_eq!(
- visitor.inner.strings,
- vec![
- "pre_visit Projection",
- "pre_visit Filter",
- "pre_visit TableScan",
- "post_visit TableScan",
- ]
+ assert_snapshot!(
+ visitor.inner.strings.join(", "),
Review Comment:
nit, if you want to remove manual join, you can do
```rust
assert_debug_snapshot!(
visitor.inner.strings,
@r#"
[
"pre_visit Projection",
"pre_visit Filter",
]
"#
);
}
```
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -2269,11 +2270,11 @@ mod tests {
.project(vec![col("id")])?
.build()?;
- let expected = "Projection: employee_csv.id\
- \n Filter: employee_csv.state = Utf8(\"CO\")\
- \n TableScan: employee_csv projection=[id, state]";
-
- assert_eq!(expected, format!("{plan}"));
+ assert_snapshot!(format!("{plan}"), @r#"
Review Comment:
Here and in other places: you can just do
```suggestion
assert_snapshot!(plan, @r#"
```
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -2498,19 +2495,8 @@ mod tests {
.project(vec![col("id"), col("first_name").alias("id")]);
match plan {
- Err(DataFusionError::SchemaError(
- SchemaError::AmbiguousReference {
- field:
- Column {
- relation: Some(TableReference::Bare { table }),
- name,
- spans: _,
- },
- },
- _,
- )) => {
- assert_eq!(*"employee_csv", *table);
- assert_eq!("id", &name);
Review Comment:
i feel like this test was slightly different - it was asserting specific
properties of the error - maybe we should keep it too?
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4059,120 +4060,118 @@ mod tests {
.project(vec![col("id"), exists(plan1).alias("exists")])?
.build();
- let expected = "Projection: employee_csv.id, EXISTS (<subquery>) AS
exists\
- \n Subquery:\
- \n TableScan: employee_csv projection=[state]\
- \n TableScan: employee_csv projection=[id, state]";
-
- assert_eq!(expected, format!("{}", plan?.display_indent()));
+ assert_snapshot!(format!("{}", plan?.display_indent()), @r"
+ Projection: employee_csv.id, EXISTS (<subquery>) AS exists
+ Subquery:
+ TableScan: employee_csv projection=[state]
+ TableScan: employee_csv projection=[id, state]
+ ");
Ok(())
}
#[test]
fn test_display_graphviz() -> Result<()> {
let plan = display_plan()?;
- let expected_graphviz = r#"
-// Begin DataFusion GraphViz Plan,
-// display it online here: https://dreampuf.github.io/GraphvizOnline
-
-digraph {
- subgraph cluster_1
- {
- graph[label="LogicalPlan"]
- 2[shape=box label="Projection: employee_csv.id"]
- 3[shape=box label="Filter: employee_csv.state IN (<subquery>)"]
- 2 -> 3 [arrowhead=none, arrowtail=normal, dir=back]
- 4[shape=box label="Subquery:"]
- 3 -> 4 [arrowhead=none, arrowtail=normal, dir=back]
- 5[shape=box label="TableScan: employee_csv projection=[state]"]
- 4 -> 5 [arrowhead=none, arrowtail=normal, dir=back]
- 6[shape=box label="TableScan: employee_csv projection=[id, state]"]
- 3 -> 6 [arrowhead=none, arrowtail=normal, dir=back]
- }
- subgraph cluster_7
- {
- graph[label="Detailed LogicalPlan"]
- 8[shape=box label="Projection: employee_csv.id\nSchema: [id:Int32]"]
- 9[shape=box label="Filter: employee_csv.state IN (<subquery>)\nSchema:
[id:Int32, state:Utf8]"]
- 8 -> 9 [arrowhead=none, arrowtail=normal, dir=back]
- 10[shape=box label="Subquery:\nSchema: [state:Utf8]"]
- 9 -> 10 [arrowhead=none, arrowtail=normal, dir=back]
- 11[shape=box label="TableScan: employee_csv projection=[state]\nSchema:
[state:Utf8]"]
- 10 -> 11 [arrowhead=none, arrowtail=normal, dir=back]
- 12[shape=box label="TableScan: employee_csv projection=[id,
state]\nSchema: [id:Int32, state:Utf8]"]
- 9 -> 12 [arrowhead=none, arrowtail=normal, dir=back]
- }
-}
-// End DataFusion GraphViz Plan
-"#;
-
// just test for a few key lines in the output rather than the
// whole thing to make test maintenance easier.
let graphviz = format!("{}", plan.display_graphviz());
- assert_eq!(expected_graphviz, graphviz);
+ assert_snapshot!(graphviz, @r#"
+ // Begin DataFusion GraphViz Plan,
+ // display it online here: https://dreampuf.github.io/GraphvizOnline
+
+ digraph {
+ subgraph cluster_1
+ {
+ graph[label="LogicalPlan"]
+ 2[shape=box label="Projection: employee_csv.id"]
+ 3[shape=box label="Filter: employee_csv.state IN (<subquery>)"]
+ 2 -> 3 [arrowhead=none, arrowtail=normal, dir=back]
+ 4[shape=box label="Subquery:"]
+ 3 -> 4 [arrowhead=none, arrowtail=normal, dir=back]
+ 5[shape=box label="TableScan: employee_csv projection=[state]"]
+ 4 -> 5 [arrowhead=none, arrowtail=normal, dir=back]
+ 6[shape=box label="TableScan: employee_csv projection=[id, state]"]
+ 3 -> 6 [arrowhead=none, arrowtail=normal, dir=back]
+ }
+ subgraph cluster_7
+ {
+ graph[label="Detailed LogicalPlan"]
+ 8[shape=box label="Projection: employee_csv.id\nSchema:
[id:Int32]"]
+ 9[shape=box label="Filter: employee_csv.state IN
(<subquery>)\nSchema: [id:Int32, state:Utf8]"]
+ 8 -> 9 [arrowhead=none, arrowtail=normal, dir=back]
+ 10[shape=box label="Subquery:\nSchema: [state:Utf8]"]
+ 9 -> 10 [arrowhead=none, arrowtail=normal, dir=back]
+ 11[shape=box label="TableScan: employee_csv
projection=[state]\nSchema: [state:Utf8]"]
+ 10 -> 11 [arrowhead=none, arrowtail=normal, dir=back]
+ 12[shape=box label="TableScan: employee_csv projection=[id,
state]\nSchema: [id:Int32, state:Utf8]"]
+ 9 -> 12 [arrowhead=none, arrowtail=normal, dir=back]
+ }
+ }
+ // End DataFusion GraphViz Plan
+ "#);
Ok(())
}
#[test]
fn test_display_pg_json() -> Result<()> {
let plan = display_plan()?;
- let expected_pg_json = r#"[
- {
- "Plan": {
- "Expressions": [
- "employee_csv.id"
- ],
- "Node Type": "Projection",
- "Output": [
- "id"
- ],
- "Plans": [
- {
- "Condition": "employee_csv.state IN (<subquery>)",
- "Node Type": "Filter",
- "Output": [
- "id",
- "state"
- ],
- "Plans": [
- {
- "Node Type": "Subquery",
+ let pg_json = format!("{}", plan.display_pg_json());
+
+ assert_snapshot!(pg_json, @r#"
Review Comment:
```suggestion
assert_snapshot!(plan.display_pg_json(), @r#"
```
--
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
