fix: don't extract common sub expr in `CASE WHEN` clause by haohuaijin · Pull Request #8833 · apache/datafusion

haohuaijin · 2024-01-11T13:27:59Z

Which issue does this PR close?

Rationale for this change

as shown in #8814, in CASE WHEN A THEN B ELSE C END expr, the B and C only have one can run,
so for query

select 
case when B.column1 > 0 then A.column1/B.column1 else 0 end as value1,
case when B.column1 > 0 and B.column2 > 0 then A.column1/B.column1 else 0 end as value3
from (select column1, column2 from users) as A ,  (select column1, column2 from users where column1 = 0) as B;

the A.column1/B.column1 actually should not run, but we extract it as a common sub expr(see below plan), which results in A.column1/B.column1 running and getting a divide zero error.

| initial_logical_plan                                       | Projection: CASE WHEN b.column1 > Int64(0) THEN a.column1 / b.column1 ELSE Int64(0) END AS value1, CASE WHEN b.column2 > Int64(0) THEN a.column1 / b.column1 ELSE Int64(0) END AS value3                                                                                                                                                        |
|                                                            |   CrossJoin:                                                                                                                                                                                                                                                                                                                                    |
|                                                            |     SubqueryAlias: a                                                                                                                                                                                                                                                                                                                            |
|                                                            |       Projection: users.column1, users.column2                                                                                                                                                                                                                                                                                                  |
|                                                            |         TableScan: users                                                                                                                                                                                                                                                                                                                        |
|                                                            |     SubqueryAlias: b                                                                                                                                                                                                                                                                                                                            |
|                                                            |       Projection: users.column1, users.column2                                                                                                                                                                                                                                                                                                  |
|                                                            |         Filter: users.column1 = Int64(0)                                                                                                                                                                                                                                                                                                        |
|                                                            |           TableScan: users                                                                                                                                                                                                                                                                                                                      |                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           |
| logical_plan after common_sub_expression_eliminate         | Projection: CASE WHEN b.column1 > Int64(0) THEN a.column1 / b.column1b.column1a.column1 AS a.column1 / b.column1 ELSE Int64(0) END AS value1, CASE WHEN b.column2 > Int64(0) THEN a.column1 / b.column1b.column1a.column1 AS a.column1 / b.column1 ELSE Int64(0) END AS value3                                                                  |
|                                                            |   Projection: a.column1 / b.column1 AS a.column1 / b.column1b.column1a.column1, a.column1, a.column2, b.column1, b.column2                                                                                                                                                                                                                      |
|                                                            |     CrossJoin:                                                                                                                                                                                                                                                                                                                                  |
|                                                            |       SubqueryAlias: a                                                                                                                                                                                                                                                                                                                          |
|                                                            |         Projection: users.column1, users.column2                                                                                                                                                                                                                                                                                                |
|                                                            |           TableScan: users                                                                                                                                                                                                                                                                                                                      |
|                                                            |       SubqueryAlias: b                                                                                                                                                                                                                                                                                                                          |
|                                                            |         Projection: users.column1, users.column2                                                                                                                                                                                                                                                                                                |
|                                                            |           Filter: users.column1 = Int64(0)                                                                                                                                                                                                                                                                                                      |
|                                                            |             TableScan: users

What changes are included in this PR?

Are these changes tested?

yes, add a test.

Are there any user-facing changes?

haohuaijin · 2024-01-11T13:28:24Z

cc @liukun4515

haohuaijin · 2024-01-11T13:32:34Z

datafusion/optimizer/src/common_subexpr_eliminate.rs

+    fn pre_visit(&mut self, expr: &Expr) -> Result<VisitRecursion> {
+        // related to https://github.com/apache/arrow-datafusion/issues/8814
+        // If the expr contain volatile expression or is a case expression, skip it.
+        if matches!(expr, Expr::Case(..)) || is_volatile_expression(expr)? {


Check volatile expression is this place is because the previous method only can deal with a single random() function, the below query wil return true in main branch

SELECT r FROM (SELECT r1 == r2 r, r1, r2 FROM (SELECT random()+1 r1, random()+1 r2) WHERE r1 > 0 AND r2 > 0)

haohuaijin · 2024-01-12T02:54:56Z

datafusion/optimizer/src/utils.rs

 }

+/// check whether the expression is volatile predicates
+pub(crate) fn is_volatile_expression(e: &Expr) -> Result<bool> {


move is_volatile_expression to utils for reuse.

liukun4515 · 2024-01-12T03:51:41Z

cc @liukun4515

thanks @haohuaijin I will help to review this pr this week.

liukun4515 · 2024-01-15T08:29:20Z

datafusion/optimizer/src/utils.rs

+            VisitRecursion::Continue
+        })
+    })
+    .unwrap();


should we use the unwrap? or the ?

because the result of is_volatile_expression is result

Thanks for pointing this out. I forgot to change it when I moved the code.

liukun4515 · 2024-01-15T08:48:53Z

datafusion/sqllogictest/test_files/select.slt

 statement ok
 DROP TABLE t;
+
+# related to https://github.com/apache/arrow-datafusion/issues/8814


liukun4515 · 2024-01-15T09:01:47Z

@jackwener could you please help to take a look this issue?

jackwener · 2024-01-15T09:18:03Z

datafusion/optimizer/src/common_subexpr_eliminate.rs

+    fn pre_visit(&mut self, expr: &Expr) -> Result<VisitRecursion> {
+        // related to https://github.com/apache/arrow-datafusion/issues/8814
+        // If the expr contain volatile expression or is a case expression, skip it.
+        if matches!(expr, Expr::Case(..)) || is_volatile_expression(expr)? {


I think this problem could exist in other function/expression like COALESCE | OR

It can be tracked as a future ticket.

The inner expressions in these expressions may be short-circuited.

track in #8874

I comment in the #8874, Do we have any method for this rule to make sure the new function/expr or other cases we can't find will not bring the same bug

jackwener

In general, look great to me, but some details need improve.

Thanks @haohuaijin & @liukun4515

jackwener · 2024-01-15T09:40:51Z

datafusion/optimizer/src/push_down_filter.rs

                        })
-                        .partition(|(_, value)| is_volatile_expression(value));
+                        .partition(|(_, value)| {
+                            is_volatile_expression(value).unwrap_or(true)


why use unwrap_or(true)?

If we don't know whether the scalar function is volatile, default set it to a volatile function.

haohuaijin · 2024-01-15T11:38:14Z

Thanks @jackwener and @liukun4515 for reviews.

liukun4515 · 2024-01-16T02:16:10Z

Thanks for @haohuaijin to fix this issue.
Thanks for @jackwener to help review this and point out other cases/issues.

liukun4515 · 2024-01-16T02:16:24Z

@alamb i will merge this pr.

* fix: don't extract common sub expr in CASE WHEN clause * fix ci * fix

… to v34 (#227) * fix: don't extract common sub expr in `CASE WHEN` clause (apache#8833) * fix: don't extract common sub expr in CASE WHEN clause * fix ci * fix * fix: common_subexpr_eliminate rule should not apply to short-circuit expression (apache#8928) * fix: common_subexpr_eliminate rule should not apply to short-circuit expression * add more tests * format * minor * apply reviews * add some commont * fmt --------- Co-authored-by: Huaijin <[email protected]>

fix: don't extract common sub expr in CASE WHEN clause

c861f40

github-actions bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Jan 11, 2024

haohuaijin commented Jan 11, 2024

View reviewed changes

fix ci

e6377c5

haohuaijin commented Jan 12, 2024

View reviewed changes

alamb mentioned this pull request Jan 12, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 8, 2024 #8786

Closed

7 tasks

alamb requested a review from liukun4515 January 12, 2024 22:27

andygrove mentioned this pull request Jan 14, 2024

Release DataFusion 35.0.0 #8863

Closed

3 tasks

alamb mentioned this pull request Jan 14, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 15, 2024 #8864

Closed

9 tasks

liukun4515 reviewed Jan 15, 2024

View reviewed changes

jackwener self-requested a review January 15, 2024 09:05

jackwener reviewed Jan 15, 2024

View reviewed changes

jackwener approved these changes Jan 15, 2024

View reviewed changes

jackwener reviewed Jan 15, 2024

View reviewed changes

haohuaijin mentioned this pull request Jan 15, 2024

Shouldn't extract common sub expression for short-circuited operation. #8874

Closed

fix

fd3e5ff

liukun4515 approved these changes Jan 16, 2024

View reviewed changes

liukun4515 merged commit 4cde998 into apache:main Jan 16, 2024

haohuaijin deleted the fix_case branch January 16, 2024 02:25

alamb mentioned this pull request Jan 19, 2024

Evaluates CASE branches even if their WHEN clause is false #8909

Closed

joroKr21 pushed a commit to coralogix/arrow-datafusion that referenced this pull request Apr 4, 2024

fix: don't extract common sub expr in CASE WHEN clause (apache#8833)

2d8d88b

* fix: don't extract common sub expr in CASE WHEN clause * fix ci * fix

Conversation

haohuaijin commented Jan 11, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

haohuaijin commented Jan 11, 2024

Uh oh!

haohuaijin Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liukun4515 commented Jan 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liukun4515 commented Jan 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackwener left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haohuaijin commented Jan 15, 2024

Uh oh!

liukun4515 commented Jan 16, 2024

Uh oh!

liukun4515 commented Jan 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

haohuaijin Jan 11, 2024 •

edited

Loading

jackwener left a comment •

edited

Loading