fix: common_subexpr_eliminate rule should not apply to short-circuit expression by haohuaijin · Pull Request #8928 · apache/datafusion

haohuaijin · 2024-01-21T07:46:30Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

before this pr

❯ explain select coalesce(1, y/x), coalesce(2, y/x) from t;
+---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type     | plan                                                                                                                                                                                                  |
+---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| logical_plan  | Projection: coalesce(Int64(1), CAST(t.y / t.x AS Int64)t.y / t.xt.xt.y AS t.y / t.x), coalesce(Int64(2), CAST(t.y / t.x AS Int64)t.y / t.xt.xt.y AS t.y / t.x)                                        |
|               |   Projection: CAST(t.y / t.x AS Int64) AS CAST(t.y / t.x AS Int64)t.y / t.xt.xt.y                                                                                                                     |
|               |     TableScan: t projection=[x, y]                                                                                                                                                                    |
| physical_plan | ProjectionExec: expr=[coalesce(1, CAST(t.y / t.x AS Int64)t.y / t.xt.xt.y@0) as coalesce(Int64(1),t.y / t.x), coalesce(2, CAST(t.y / t.x AS Int64)t.y / t.xt.xt.y@0) as coalesce(Int64(2),t.y / t.x)] |
|               |   ProjectionExec: expr=[CAST(y@1 / x@0 AS Int64) as CAST(t.y / t.x AS Int64)t.y / t.xt.xt.y]                                                                                                          |
|               |     MemoryExec: partitions=1, partition_sizes=[1]                                                                                                                                                     |
|               |                                                                                                                                                                                                       |
+---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
2 rows in set. Query took 0.014 seconds.

after this pr

❯ explain select coalesce(1, y/x), coalesce(2, y/x) from t;
+---------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type     | plan                                                                                                                                                                |
+---------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| logical_plan  | Projection: coalesce(Int64(1), CAST(t.y / t.x AS Int64)), coalesce(Int64(2), CAST(t.y / t.x AS Int64))                                                              |
|               |   TableScan: t projection=[x, y]                                                                                                                                    |
| physical_plan | ProjectionExec: expr=[coalesce(1, CAST(y@1 / x@0 AS Int64)) as coalesce(Int64(1),t.y / t.x), coalesce(2, CAST(y@1 / x@0 AS Int64)) as coalesce(Int64(2),t.y / t.x)] |
|               |   MemoryExec: partitions=1, partition_sizes=[1]                                                                                                                     |
|               |                                                                                                                                                                     |
+---------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------+
2 rows in set. Query took 0.008 seconds.

But due to the reason describe in #8927, the above two query will get same result(get drive by zero error).

Are these changes tested?

yes, add some negative test

Are there any user-facing changes?

…expression

haohuaijin · 2024-01-21T07:48:44Z

datafusion/sqllogictest/test_files/select.slt

 0 0

+query TT
+explain select coalesce(1, y/x), coalesce(2, y/x) from t;


because of the reason describe in #8927, use plan to test.

Can we please add some comments here explaining the rationale and what the expected outputs are (so future readers know if changes are expected)

Something like this perhaps:

# Expressions that short circuit should not be refactored out as that may cause side effects (divide by zero) # at plan time that would not actually happen during execution

Also, can you please add the tests that now pass (e.g. select coalesce(1, y/x), coalesce(2, y/x) from t;) so if someone breaks this code by accident, those queries would start failing, which might be easier to quickly tell is incorrect

haohuaijin · 2024-01-21T08:07:59Z

cc @jackwener @liukun4515 @alamb

alamb

Thank you for this PR @haohuaijin -- the code looks good to me 🙏

I also took a look at https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.BuiltinScalarFunction.html# and https://docs.rs/datafusion/latest/datafusion/logical_expr/expr/enum.Expr.html and didn't see any additional functions that shoudl be marked as short circuiting.

I had a code structure suggestion that I don't think is needed (though I think it would improve this PR).

I think a few more tests that actually run (and don't error) should be added prior to merge.

Thanks again!

datafusion/optimizer/src/common_subexpr_eliminate.rs

alamb · 2024-01-21T14:34:37Z

datafusion/sqllogictest/test_files/select.slt

 0 0

+query TT
+explain select coalesce(1, y/x), coalesce(2, y/x) from t;


Can we please add some comments here explaining the rationale and what the expected outputs are (so future readers know if changes are expected)

Something like this perhaps:

# Expressions that short circuit should not be refactored out as that may cause side effects (divide by zero) # at plan time that would not actually happen during execution

Also, can you please add the tests that now pass (e.g. select coalesce(1, y/x), coalesce(2, y/x) from t;) so if someone breaks this code by accident, those queries would start failing, which might be easier to quickly tell is incorrect

jackwener

Thanks @haohuaijin @alamb .

@alamb 's review suggestions are already very detailed, and I agree with his opinion. After those impromentent, we can merge this PR

haohuaijin · 2024-01-22T08:13:21Z

Thanks @alamb and @jackwener's review and great suggestions.

I apply those suggestions, but I'm unsure if the test I added is appropriate(560cb6c).

I also add describe in #8928 (comment), to show what the change in this pr.

haohuaijin · 2024-01-22T08:16:20Z

datafusion/sqllogictest/test_files/select.slt

+ProjectionExec: expr=[y@1 = 0 OR 1 / CAST(y@1 AS Int64) < 1 as t.y = Int64(0) OR Int64(1) / t.y < Int64(1), x@0 = 0 OR y@1 = 0 OR 1 / CAST(y@1 AS Int64) < 1 / CAST(x@0 AS Int64) as t.x = Int64(0) OR t.y = Int64(0) OR Int64(1) / t.y < Int64(1) / t.x]
+--MemoryExec: partitions=1, partition_sizes=[1]
+
+# due to the reason describe in https://github.com/apache/arrow-datafusion/issues/8927,


I'm unsure if those tests are appropriate.

It's related with #8910

alamb

Looks good to me -- thank you @haohuaijin

alamb · 2024-01-22T19:04:15Z

datafusion/expr/src/expr.rs

+                matches!(op, Operator::And | Operator::Or)
+            }
+            Expr::Case { .. } => true,
+            // Use explicit pattern match instead of a default


alamb · 2024-01-22T19:16:37Z

Thank you @haohuaijin and @jackwener -- let's merge this PR and we can work on #8910 as a follow on

…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

… 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]>

haohuaijin added 4 commits January 21, 2024 12:50

fix: common_subexpr_eliminate rule should not apply to short-circuit …

2c30683

…expression

add more tests

584f5c8

format

7329b91

minor

f89a4d1

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

haohuaijin commented Jan 21, 2024

View reviewed changes

alamb mentioned this pull request Jan 21, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 22, 2024 #8933

Closed

9 tasks

alamb reviewed Jan 21, 2024

View reviewed changes

alamb mentioned this pull request Jan 21, 2024

Evaluates COALESCE arguments past first non-NULL value #8910

Closed

jackwener approved these changes Jan 22, 2024

View reviewed changes

apply reviews

560cb6c

github-actions bot added the logical-expr Logical plan and expressions label Jan 22, 2024

haohuaijin added 2 commits January 22, 2024 16:06

add some commont

fcb37cc

fmt

b3f50c0

haohuaijin commented Jan 22, 2024

View reviewed changes

haohuaijin requested review from alamb and jackwener January 22, 2024 08:17

alamb approved these changes Jan 22, 2024

View reviewed changes

alamb merged commit c9935ae into apache:main Jan 22, 2024

haohuaijin deleted the fix branch January 23, 2024 00:27

haohuaijin restored the fix branch January 23, 2024 04:37

haohuaijin deleted the fix branch January 23, 2024 04:41

alamb mentioned this pull request Jul 2, 2024

Enhance short circuit handling in CommonSubexprEliminate #11197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: common_subexpr_eliminate rule should not apply to short-circuit expression#8928

fix: common_subexpr_eliminate rule should not apply to short-circuit expression#8928
alamb merged 7 commits intoapache:mainfrom
haohuaijin:fix

haohuaijin commented Jan 21, 2024 •

edited

Loading

Uh oh!

haohuaijin Jan 21, 2024

Uh oh!

alamb Jan 21, 2024

Uh oh!

haohuaijin commented Jan 21, 2024

Uh oh!

alamb left a comment •

edited

Loading

Uh oh!

Uh oh!

alamb Jan 21, 2024

Uh oh!

jackwener left a comment

Uh oh!

haohuaijin commented Jan 22, 2024 •

edited

Loading

Uh oh!

haohuaijin Jan 22, 2024

Uh oh!

jackwener Jan 22, 2024

Uh oh!

alamb left a comment

Uh oh!

alamb Jan 22, 2024

Uh oh!

alamb commented Jan 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

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 Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

alamb Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

haohuaijin commented Jan 21, 2024

Uh oh!

alamb 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!

Uh oh!

alamb Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

jackwener left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

haohuaijin Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

jackwener Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

alamb commented Jan 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

haohuaijin commented Jan 21, 2024 •

edited

Loading

alamb left a comment •

edited

Loading

haohuaijin commented Jan 22, 2024 •

edited

Loading