Improve volatile expression handling in `CommonSubexprEliminate` by peter-toth · Pull Request #11265 · apache/datafusion

peter-toth · 2024-07-04T12:22:11Z

Which issue does this PR close?

Part of #11194.

Rationale for this change

Curently CommonSubexprEliminate always skips expressions doesn't allow volatile expressions to be extracted, but this can be improved by allowing non-volatile subexpressions of volatile expressions to be extracted:

I.e. from the plan:

Projection (a + b) + rand() AS c1, (a + b) + rand() AS c2

the expression a + b can be extracted as common expression despite the fact that (a + b) + rand() is volatile.

What changes are included in this PR?

Changes the rule to handle volatile expressions better by adding a new boolean flag to VisitRecord::ExprItem and propagating up volatility (validity for CSE).

Are these changes tested?

Yes, added new UT.

Are there any user-facing changes?

No.

peter-toth · 2024-07-04T13:06:51Z

cc @alamb, this the 2nd PR of #11194 and removes a TODO.

alamb

Thank you @peter-toth -- this looks quite cool. I had one question about the test and some comment suggestions. The test is the only thing I think is needed before merging this PR

cc @waynexia and @haohuaijin in case you have comments

It may also be worth writing a few .slt tests showing this in action and how it interacts with multiple subexpressions

alamb · 2024-07-05T16:37:20Z


 impl<'n> ExprIdentifierVisitor<'_, 'n> {
 /// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
 /// before it.


Could you possibly update this doc string to explain the 4-tuple that is now returned? Specifically document the two different bool values?

Sure, good idea, I've added the comment in 79b2e02

alamb · 2024-07-05T16:39:09Z

+ let plan = LogicalPlanBuilder::from(table_scan.clone())
+ .project(vec![
+ not_extracted_volatile_short_circuit_1.clone().alias("c1"),
+ not_extracted_volatile_short_circuit_1.alias("c2"),


🤔 I was thinking -- why can't we extract the a = 0 part of a = 0 OR random() = 0 out?
It seems like it would be ok to rewrite
SELECT a =0OR random() =0as c1, a =0OR random() =0as c2, ..
to something like this:
SELECT __subexpr_1 OR random() as c1, __subexpr_1 OR random() as c2, ... FROM ( SELECT a=0as __subexpr_1 FROM ...

This question is similar to #11197 (comment). We can extract the surely evaluated expressions (1st legs) from those short circuiting Or, And and CaseWhen expressions, but we are not there yet with this PR.
My 3rd PR in the #11194 epic will implement that logic, but I want to add the improvements gradually as it will require to recurse into only certain children of a parent and that's not straightforward with the current treenode APIs (i.e. we need to stop recursion at the parent with a Jump and start a new recursion only on the interresting children).

alamb · 2024-07-05T16:44:55Z

- fn pop_enter_mark(&mut self) -> (usize, bool, Option<Identifier<'n>>) {
+ fn pop_enter_mark(&mut self) -> (usize, bool, Option<Identifier<'n>>, bool) {
 let mut expr_id = None;
+ let mut is_valid = true;


Could you please document what "valid" means in this context? I think it means "is valid for CSE" as in "this sub expression could potentially be removed via CSE" but I am not quite sure

I added a comment to the method in 79b2e02, I think it explains what "valid" means in this case, but let me know if we should document the variable as well.

peter-toth · 2024-07-05T17:51:49Z

It may also be worth writing a few .slt tests showing this in action and how it interacts with multiple subexpressions

Sure, I will add some tests tomorrow.

haohuaijin

Looks good to me, thanks @peter-toth and @alamb.

peter-toth · 2024-07-06T09:12:17Z

I've added CSE tests in 1aa8848, it contais TODOs for #11265 (comment), but I will fix them in the next PR.

…handling

alamb

Thank you @peter-toth and @haohuaijin

I merged up from main and updated the unit test to avoid a new dependency on datafusion-functions.

…handling

alamb · 2024-07-08T16:42:49Z

Thanks again @peter-toth and @haohuaijin

peter-toth · 2024-07-08T18:43:25Z

Thanks @alamb and @haohuaijin for the review!

…che#11265) * Improve volatile expression handling in `CommonSubexprEliminate` rule * fix volatile handling with short circuits * fix comments * add slt tests for CSE * Avoid adding datafusion function dependency * revert changes to datafusion-cli.lock --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

peter-toth changed the title ~~Improve volatile expression handling in CommonSubexprEliminate rule~~Improve volatile expression handling in CommonSubexprEliminateJul 4, 2024

Improve volatile expression handling in CommonSubexprEliminate rule
6532b97

peter-toth force-pushed the improve-cse-volatile-handling branch from 4b395e1 to 6532b97Compare July 4, 2024 12:31

github-actionsBot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Jul 4, 2024

fix volatile handling with short circuits
3c3802c

alamb reviewed Jul 5, 2024
View reviewed changes

fix comments
79b2e02

haohuaijin approved these changes Jul 6, 2024
View reviewed changes

add slt tests for CSE
1aa8848

github-actionsBot added the sqllogictest SQL Logic Tests (.slt) label Jul 6, 2024

alamb added 2 commits July 6, 2024 07:22

Merge remote-tracking branch 'apache/main' into improve-cse-volatile-…
4a0425a
…handling

Avoid adding datafusion function dependency
21917ed

alamb approved these changes Jul 6, 2024
View reviewed changes

alamb added 2 commits July 6, 2024 07:51

revert changes to datafusion-cli.lock
0f3932d

Merge remote-tracking branch 'apache/main' into improve-cse-volatile-…
ff32440
…handling

alamb merged commit b6281b5 into apache:mainJul 8, 2024

peter-toth mentioned this pull request Jul 9, 2024
Improve CommonSubexprEliminate rule with surely and conditionally evaluated stats #11357
Merged

Uh oh!

Conversation

peter-toth commented Jul 4, 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!

peter-toth commented Jul 4, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alambJul 5, 2024

Choose a reason for hiding this comment

Uh oh!

peter-tothJul 5, 2024

Choose a reason for hiding this comment

Uh oh!

alambJul 5, 2024

Choose a reason for hiding this comment

Uh oh!

peter-tothJul 5, 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!

alambJul 5, 2024

Choose a reason for hiding this comment

Uh oh!

peter-tothJul 5, 2024

Choose a reason for hiding this comment

Uh oh!

peter-toth commented Jul 5, 2024

Uh oh!

haohuaijin left a comment

Choose a reason for hiding this comment

Uh oh!

peter-toth commented Jul 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Jul 8, 2024

Uh oh!

peter-toth commented Jul 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peter-toth commented Jul 4, 2024•
edited
Loading

peter-tothJul 5, 2024•
edited
Loading

peter-toth commented Jul 6, 2024•
edited
Loading