alamb commented on code in PR #11683:
URL: https://github.com/apache/datafusion/pull/11683#discussion_r1697491116
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -1963,6 +1944,52 @@ mod test {
Ok(())
}
+ #[test]
+ fn test_non_top_level_common_expression() -> Result<()> {
Review Comment:
I verified without the code chages in this PR this test fails like this:
```
failed to optimize plan
thread
'common_subexpr_eliminate::test::test_non_top_level_common_expression' panicked
at datafusion/optimizer/src/common_subexpr_eliminate.rs:1215:9:
failed to optimize plan
stack backtrace:
0: rust_begin_unwind
at
/rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:652:5
1: core::panicking::panic_fmt
at
/rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/panicking.rs:72:14
2:
datafusion_optimizer::common_subexpr_eliminate::test::assert_optimized_plan_eq
at ./src/common_subexpr_eliminate.rs:1215:9
3:
datafusion_optimizer::common_subexpr_eliminate::test::test_non_top_level_common_expression
at ./src/common_subexpr_eliminate.rs:1984:9
4:
datafusion_optimizer::common_subexpr_eliminate::test::test_non_top_level_common_expression::{{closure}}
at ./src/common_subexpr_eliminate.rs:1967:50
5: core::ops::function::FnOnce::call_once
at
/rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/ops/function.rs:250:5
6: core::ops::function::FnOnce::call_once
at
/rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose
backtrace.
```
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -1963,6 +1944,52 @@ mod test {
Ok(())
}
+ #[test]
+ fn test_non_top_level_common_expression() -> Result<()> {
+ let table_scan = test_table_scan()?;
+
+ let common_expr = col("a") + col("b");
+ let plan = LogicalPlanBuilder::from(table_scan)
+ .project(vec![
+ common_expr.clone().alias("c1"),
+ common_expr.alias("c2"),
+ ])?
+ .project(vec![col("c1"), col("c2")])?
+ .build()?;
+
+ let expected = "Projection: c1, c2\
+ \n Projection: __common_expr_1 AS c1, __common_expr_1 AS c2\
+ \n Projection: test.a + test.b AS __common_expr_1, test.a, test.b,
test.c\
+ \n TableScan: test";
+
+ assert_optimized_plan_eq(expected, plan, None);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_nested_common_expression() -> Result<()> {
Review Comment:
I verified this test fails without the changes in this PR:
```
assertion `left == right` failed
left: "Projection: __common_expr_1 AS c1, __common_expr_1 AS c2\n
Projection: __common_expr_2 * __common_expr_2 AS __common_expr_1, test.a,
test.b, test.c\n Projection: test.a + test.b AS __common_expr_2, test.a,
test.b, test.c\n TableScan: test"
right: "Projection: __common_expr_1 AS c1, __common_expr_1 AS c2\n
Projection: (test.a + test.b) * (test.a + test.b) AS __common_expr_1, test.a,
test.b, test.c\n TableScan: test"
<Click to see difference>
thread 'common_subexpr_eliminate::test::test_nested_common_expression'
panicked at datafusion/optimizer/src/common_subexpr_eliminate.rs:1218:9:
assertion `left == right` failed
left: "Projection: __common_expr_1 AS c1, __common_expr_1 AS c2\n
Projection: __common_expr_2 * __common_expr_2 AS __common_expr_1, test.a,
test.b, test.c\n Projection: test.a + test.b AS __common_expr_2, test.a,
test.b, test.c\n TableScan: test"
right: "Projection: __common_expr_1 AS c1, __common_expr_1 AS c2\n
Projection: (test.a + test.b) * (test.a + test.b) AS __common_expr_1, test.a,
test.b, test.c\n TableScan: test"
stack backtrace:
0: rust_begin_unwind
at
/rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:652:5
1: core::panicking::panic_fmt
at
/rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/panicking.rs:72:14
2: core::panicking::assert_failed_inner
3: core::panicking::assert_failed
at
/rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/panicking.rs:363:5
4:
datafusion_optimizer::common_subexpr_eliminate::test::assert_optimized_plan_eq
at ./src/common_subexpr_eliminate.rs:1218:9
5:
datafusion_optimizer::common_subexpr_eliminate::test::test_nested_common_expression
at ./src/common_subexpr_eliminate.rs:2007:9
6:
datafusion_optimizer::common_subexpr_eliminate::test::test_nested_common_expression::{{closure}}
at ./src/common_subexpr_eliminate.rs:1990:43
7: core::ops::function::FnOnce::call_once
at
/rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/ops/function.rs:250:5
8: core::ops::function::FnOnce::call_once
at
/rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose
backtrace.
```
Namely the output projection looks like
```
Projection: __common_expr_1 AS c1, __common_expr_1 AS c2
Projection: (test.a + test.b) * (test.a + test.b) AS __common_expr_1,
test.a, test.b, test.c
TableScan: test
```
Rather than
```
Projection: __common_expr_1 AS c1, __common_expr_1 AS c2
Projection: __common_expr_2 * __common_expr_2 AS __common_expr_1, test.a,
test.b, test.c
Projection: test.a + test.b AS __common_expr_2, test.a, test.b, test.c
TableScan: test
```
--
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]
