Skip to content

Always add parentheses when formatting BinaryExpr with SchemaDisplay #16209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Always add parentheses when formatting BinaryExpr with SchemaDisplay #16209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5d3e422
Fix ScdhemaDisplay
hendrikmakait May 26, 2025
6b30b4f
Add test
hendrikmakait May 29, 2025
085bf74
linting
hendrikmakait May 29, 2025
fdbfa36
Always add parantheses
hendrikmakait May 29, 2025
e7aa204
Additional test case
hendrikmakait May 29, 2025
378b969
Adjust tests
hendrikmakait May 31, 2025
1c9c48a
more tests
hendrikmakait May 31, 2025
8b1f34c
additional tests
hendrikmakait May 31, 2025
0981c99
More tests
hendrikmakait May 31, 2025
a7769e8
sqllogictests
hendrikmakait May 31, 2025
6d85f17
Adjust example
hendrikmakait Jun 4, 2025
eab812a
Adjust TPC-H
hendrikmakait Jun 4, 2025
0e5d595
Merge branch 'main' into fix-schemadisplay-for-nested-binaryexprs
hendrikmakait Jun 4, 2025
1c73c3d
Merge branch 'main' into fix-schemadisplay-for-nested-binaryexprs
hendrikmakait Jun 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions datafusion-examples/examples/date_time_functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,15 @@ async fn query_make_date() -> Result<()> {
let df = ctx.sql("select make_date(y + 1, m, d) from t").await?;

let expected = [
"+-----------------------------------+",
"| make_date(t.y + Int64(1),t.m,t.d) |",
"+-----------------------------------+",
"| 2021-01-15 |",
"| 2022-02-16 |",
"| 2023-03-17 |",
"| 2024-04-18 |",
"| 2025-05-19 |",
"+-----------------------------------+",
"+-------------------------------------+",
"| make_date((t.y + Int64(1)),t.m,t.d) |",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parens here seem unecessary (the top most expression) -- I wonder if we can avoid that somehow

"+-------------------------------------+",
"| 2021-01-15 |",
"| 2022-02-16 |",
"| 2023-03-17 |",
"| 2024-04-18 |",
"| 2025-05-19 |",
"+-------------------------------------+",
];

assert_batches_eq!(expected, &df.collect().await?);
Expand Down
10 changes: 5 additions & 5 deletions datafusion/core/src/dataframe/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,11 +358,11 @@ impl DataFrame {
/// let df = ctx.read_csv("tests/data/example.csv", CsvReadOptions::new()).await?;
/// let df = df.select(vec![col("a"), col("b") * col("c")])?;
/// let expected = vec![
/// "+---+-----------------------+",
/// "| a | ?table?.b * ?table?.c |",
/// "+---+-----------------------+",
/// "| 1 | 6 |",
/// "+---+-----------------------+"
/// "+---+-------------------------+",
/// "| a | (?table?.b * ?table?.c) |",
/// "+---+-------------------------+",
/// "| 1 | 6 |",
/// "+---+-------------------------+"
/// ];
/// # assert_batches_sorted_eq!(expected, &df.collect().await?);
/// # Ok(())
Expand Down
10 changes: 5 additions & 5 deletions datafusion/core/src/execution/context/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1963,11 +1963,11 @@ mod tests {
.await?;

assert_snapshot!(batches_to_string(&results), @r"
+----------------------+------------------------+---------------------+
| @@version | @name | @integer + Int64(1) |
+----------------------+------------------------+---------------------+
| system-var-@@version | user-defined-var-@name | 42 |
+----------------------+------------------------+---------------------+
+----------------------+------------------------+-----------------------+
| @@version | @name | (@integer + Int64(1)) |
+----------------------+------------------------+-----------------------+
| system-var-@@version | user-defined-var-@name | 42 |
+----------------------+------------------------+-----------------------+
");

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion datafusion/core/src/physical_planner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3042,7 +3042,7 @@ mod tests {
// display it online here: https://dreampuf.github.io/GraphvizOnline

digraph {
1[shape=box label="ProjectionExec: expr=[id@0 + 2 as employee.id + Int32(2)]", tooltip=""]
1[shape=box label="ProjectionExec: expr=[id@0 + 2 as (employee.id + Int32(2))]", tooltip=""]
2[shape=box label="EmptyExec", tooltip=""]
1 -> 2 [arrowhead=none, arrowtail=normal, dir=back]
}
Expand Down
50 changes: 25 additions & 25 deletions datafusion/core/tests/dataframe/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,15 +835,15 @@ async fn test_aggregate_subexpr() -> Result<()> {
assert_snapshot!(
batches_to_sort_string(&df_results),
@r###"
+----------------+------+
| c2 + Int32(10) | sum |
+----------------+------+
| 12 | 431 |
| 13 | 248 |
| 14 | 453 |
| 15 | 95 |
| 16 | -146 |
+----------------+------+
+------------------+------+
| (c2 + Int32(10)) | sum |
+------------------+------+
| 12 | 431 |
| 13 | 248 |
| 14 | 453 |
| 15 | 95 |
| 16 | -146 |
+------------------+------+
"###
);

Expand Down Expand Up @@ -5659,14 +5659,14 @@ async fn test_alias() -> Result<()> {
assert_snapshot!(
batches_to_sort_string(&df.collect().await.unwrap()),
@r###"
+-----------+---------------------------------+
| a | table_alias.b + table_alias.one |
+-----------+---------------------------------+
| 123AbcDef | 101 |
| CBAdef | 11 |
| abc123 | 11 |
| abcDEF | 2 |
+-----------+---------------------------------+
+-----------+-----------------------------------+
| a | (table_alias.b + table_alias.one) |
+-----------+-----------------------------------+
| 123AbcDef | 101 |
| CBAdef | 11 |
| abc123 | 11 |
| abcDEF | 2 |
+-----------+-----------------------------------+
"###
);
Ok(())
Expand Down Expand Up @@ -5769,14 +5769,14 @@ async fn test_alias_nested() -> Result<()> {
assert_snapshot!(
batches_to_sort_string(&select1.collect().await.unwrap()),
@r###"
+-----------+-----------------------+
| a | alias2.b + alias2.one |
+-----------+-----------------------+
| 123AbcDef | 101 |
| CBAdef | 11 |
| abc123 | 11 |
| abcDEF | 2 |
+-----------+-----------------------+
+-----------+-------------------------+
| a | (alias2.b + alias2.one) |
+-----------+-------------------------+
| 123AbcDef | 101 |
| CBAdef | 11 |
| abc123 | 11 |
| abcDEF | 2 |
+-----------+-------------------------+
"###
);

Expand Down
2 changes: 1 addition & 1 deletion datafusion/core/tests/expr_api/simplification.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ fn select_date_plus_interval() -> Result<()> {
assert_snapshot!(
actual,
@r###"
Projection: Date32("2021-01-09") AS to_timestamp(Utf8("2020-09-08T12:05:00+00:00")) + IntervalDayTime("IntervalDayTime { days: 123, milliseconds: 0 }")
Projection: Date32("2021-01-09") AS (to_timestamp(Utf8("2020-09-08T12:05:00+00:00")) + IntervalDayTime("IntervalDayTime { days: 123, milliseconds: 0 }"))
TableScan: test
"###
);
Expand Down
20 changes: 10 additions & 10 deletions datafusion/core/tests/user_defined/expr_planner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,23 @@ async fn plan_and_collect(sql: &str) -> Result<Vec<RecordBatch>> {
async fn test_custom_operators_arrow() {
let actual = plan_and_collect("select 'foo'->'bar';").await.unwrap();
insta::assert_snapshot!(batches_to_string(&actual), @r###"
+----------------------------+
| Utf8("foo") || Utf8("bar") |
+----------------------------+
| foobar |
+----------------------------+
+------------------------------+
| (Utf8("foo") || Utf8("bar")) |
+------------------------------+
| foobar |
+------------------------------+
"###);
}

#[tokio::test]
async fn test_custom_operators_long_arrow() {
let actual = plan_and_collect("select 1->>2;").await.unwrap();
insta::assert_snapshot!(batches_to_string(&actual), @r###"
+---------------------+
| Int64(1) + Int64(2) |
+---------------------+
| 3 |
+---------------------+
+-----------------------+
| (Int64(1) + Int64(2)) |
+-----------------------+
| 3 |
+-----------------------+
"###);
}

Expand Down
21 changes: 18 additions & 3 deletions datafusion/expr/src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,10 +1405,10 @@ impl Expr {
/// ```
/// # use datafusion_expr::{col, lit};
/// let expr = col("foo").eq(lit(42));
/// assert_eq!("foo = Int32(42)", expr.schema_name().to_string());
/// assert_eq!("(foo = Int32(42))", expr.schema_name().to_string());
///
/// let expr = col("foo").alias("bar").eq(lit(11));
/// assert_eq!("bar = Int32(11)", expr.schema_name().to_string());
/// assert_eq!("(bar = Int32(11))", expr.schema_name().to_string());
/// ```
///
/// [`Schema`]: arrow::datatypes::Schema
Expand Down Expand Up @@ -2722,7 +2722,7 @@ impl Display for SchemaDisplay<'_> {
}
}
Expr::BinaryExpr(BinaryExpr { left, op, right }) => {
write!(f, "{} {op} {}", SchemaDisplay(left), SchemaDisplay(right),)
write!(f, "({} {op} {})", SchemaDisplay(left), SchemaDisplay(right),)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual change.

}
Expr::Case(Case {
expr,
Expand Down Expand Up @@ -3820,6 +3820,21 @@ mod test {
);
}

#[test]
fn test_schema_display_nested_binary_expr() {
let expr = lit(1) * (lit(2) + lit(3));
assert_eq!(
format!("{}", SchemaDisplay(&expr)),
"(Int32(1) * (Int32(2) + Int32(3)))"
);

let expr = -(lit(1) + (lit(2)));
assert_eq!(
format!("{}", SchemaDisplay(&expr)),
"(- (Int32(1) + Int32(2)))"
);
}

fn wildcard_options(
opt_ilike: Option<IlikeSelectItem>,
opt_exclude: Option<ExcludeSelectItem>,
Expand Down
26 changes: 13 additions & 13 deletions datafusion/optimizer/src/common_subexpr_eliminate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Aggregate: groupBy=[[]], aggr=[[sum(__common_expr_1 AS test.a * Int32(1) - test.b), sum(__common_expr_1 AS test.a * Int32(1) - test.b * (Int32(1) + test.c))]]
Aggregate: groupBy=[[]], aggr=[[sum(__common_expr_1 AS (test.a * (Int32(1) - test.b))), sum(__common_expr_1 AS (test.a * (Int32(1) - test.b)) * (Int32(1) + test.c))]]
Projection: test.a * (Int32(1) - test.b) AS __common_expr_1, test.a, test.b, test.c
TableScan: test
"
Expand All @@ -884,7 +884,7 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Projection: __common_expr_1 - test.c AS alias1 * __common_expr_1 AS test.a + test.b, __common_expr_1 AS test.a + test.b
Projection: __common_expr_1 - test.c AS alias1 * __common_expr_1 AS (test.a + test.b), __common_expr_1 AS (test.a + test.b)
Projection: test.a + test.b AS __common_expr_1, test.a, test.b, test.c
TableScan: test
"
Expand Down Expand Up @@ -1000,7 +1000,7 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Aggregate: groupBy=[[__common_expr_1 AS UInt32(1) + test.a]], aggr=[[avg(__common_expr_1) AS col1, my_agg(__common_expr_1) AS col2]]
Aggregate: groupBy=[[__common_expr_1 AS (UInt32(1) + test.a)]], aggr=[[avg(__common_expr_1) AS col1, my_agg(__common_expr_1) AS col2]]
Projection: UInt32(1) + test.a AS __common_expr_1, test.a, test.b, test.c
TableScan: test
"
Expand All @@ -1024,8 +1024,8 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Projection: UInt32(1) + test.a, UInt32(1) + __common_expr_2 AS col1, UInt32(1) - __common_expr_2 AS col2, __common_expr_4 AS avg(UInt32(1) + test.a), UInt32(1) + __common_expr_3 AS col3, UInt32(1) - __common_expr_3 AS col4, __common_expr_5 AS my_agg(UInt32(1) + test.a)
Aggregate: groupBy=[[__common_expr_1 AS UInt32(1) + test.a]], aggr=[[avg(__common_expr_1) AS __common_expr_2, my_agg(__common_expr_1) AS __common_expr_3, avg(__common_expr_1 AS UInt32(1) + test.a) AS __common_expr_4, my_agg(__common_expr_1 AS UInt32(1) + test.a) AS __common_expr_5]]
Projection: (UInt32(1) + test.a), UInt32(1) + __common_expr_2 AS col1, UInt32(1) - __common_expr_2 AS col2, __common_expr_4 AS avg((UInt32(1) + test.a)), UInt32(1) + __common_expr_3 AS col3, UInt32(1) - __common_expr_3 AS col4, __common_expr_5 AS my_agg((UInt32(1) + test.a))
Aggregate: groupBy=[[__common_expr_1 AS (UInt32(1) + test.a)]], aggr=[[avg(__common_expr_1) AS __common_expr_2, my_agg(__common_expr_1) AS __common_expr_3, avg(__common_expr_1 AS (UInt32(1) + test.a)) AS __common_expr_4, my_agg(__common_expr_1 AS (UInt32(1) + test.a)) AS __common_expr_5]]
Projection: UInt32(1) + test.a AS __common_expr_1, test.a, test.b, test.c
TableScan: test
"
Expand All @@ -1052,8 +1052,8 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Projection: table.test.col.a, UInt32(1) + __common_expr_2 AS avg(UInt32(1) + table.test.col.a), __common_expr_2 AS avg(UInt32(1) + table.test.col.a)
Aggregate: groupBy=[[table.test.col.a]], aggr=[[avg(__common_expr_1 AS UInt32(1) + table.test.col.a) AS __common_expr_2]]
Projection: table.test.col.a, UInt32(1) + __common_expr_2 AS avg((UInt32(1) + table.test.col.a)), __common_expr_2 AS avg((UInt32(1) + table.test.col.a))
Aggregate: groupBy=[[table.test.col.a]], aggr=[[avg(__common_expr_1 AS (UInt32(1) + table.test.col.a)) AS __common_expr_2]]
Projection: UInt32(1) + table.test.col.a AS __common_expr_1, table.test.col.a
TableScan: table.test
"
Expand Down Expand Up @@ -1092,7 +1092,7 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Projection: __common_expr_1 AS Int32(1) + test.a, __common_expr_1 AS test.a + Int32(1)
Projection: __common_expr_1 AS (Int32(1) + test.a), __common_expr_1 AS (test.a + Int32(1))
Projection: Int32(1) + test.a AS __common_expr_1, test.a, test.b, test.c
TableScan: test
"
Expand Down Expand Up @@ -1692,7 +1692,7 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Projection: __common_expr_1 AS NOT test.a = test.b, __common_expr_1 AS NOT test.b = test.a
Projection: __common_expr_1 AS NOT (test.a = test.b), __common_expr_1 AS NOT (test.b = test.a)
Projection: NOT test.a = test.b AS __common_expr_1, test.a, test.b, test.c
TableScan: test
"
Expand All @@ -1708,7 +1708,7 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Projection: __common_expr_1 AS test.a = test.b IS NULL, __common_expr_1 AS test.b = test.a IS NULL
Projection: __common_expr_1 AS (test.a = test.b) IS NULL, __common_expr_1 AS (test.b = test.a) IS NULL
Projection: test.a = test.b IS NULL AS __common_expr_1, test.a, test.b, test.c
TableScan: test
"
Expand All @@ -1724,7 +1724,7 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Projection: __common_expr_1 AS test.a + test.b BETWEEN Int32(0) AND Int32(10), __common_expr_1 AS test.b + test.a BETWEEN Int32(0) AND Int32(10)
Projection: __common_expr_1 AS (test.a + test.b) BETWEEN Int32(0) AND Int32(10), __common_expr_1 AS (test.b + test.a) BETWEEN Int32(0) AND Int32(10)
Projection: test.a + test.b BETWEEN Int32(0) AND Int32(10) AS __common_expr_1, test.a, test.b, test.c
TableScan: test
"
Expand All @@ -1740,7 +1740,7 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Projection: __common_expr_1 AS test.c BETWEEN test.a + test.b AND Int32(10), __common_expr_1 AS test.c BETWEEN test.b + test.a AND Int32(10)
Projection: __common_expr_1 AS test.c BETWEEN (test.a + test.b) AND Int32(10), __common_expr_1 AS test.c BETWEEN (test.b + test.a) AND Int32(10)
Projection: test.c BETWEEN test.a + test.b AND Int32(10) AS __common_expr_1, test.a, test.b, test.c
TableScan: test
"
Expand All @@ -1757,7 +1757,7 @@ mod test {
assert_optimized_plan_equal!(
plan,
@ r"
Projection: __common_expr_1 AS my_udf(test.a + test.b), __common_expr_1 AS my_udf(test.b + test.a)
Projection: __common_expr_1 AS my_udf((test.a + test.b)), __common_expr_1 AS my_udf((test.b + test.a))
Projection: my_udf(test.a + test.b) AS __common_expr_1, test.a, test.b, test.c
TableScan: test
"
Expand Down
Loading