Enhance short circuit handling in CommonSubexprEliminate
#11197
Conversation
|
Thanks @peter-toth -- I plan to review this tomorrow morning |
There was a problem hiding this comment.
Thank you @peter-toth -- I spent some time this morning playing with this PR and I think it makes sense to me.
Maybe @haohuaijin (as the original author of this feature in #8928) might have some time to review this PR as well
| } | ||
| let is_tree = expr.short_circuits(); | ||
| let tnr = if is_tree { | ||
| TreeNodeRecursion::Jump |
There was a problem hiding this comment.
I don't understand the meaning of is_tree here.
Maybe we could add a comment explaining that Jump will skip children but continue with siblings
There was a problem hiding this comment.
maybe we can rename is_tree to is_short_circuits
There was a problem hiding this comment.
Oh, I forgot to add a comment here. Basically I wanted to express that we handle the expression as a subtree (not just a node) in this case.
I added a comment in c02bae9.
|
|
||
| let extracted_short_circuit = col("a").eq(lit(0)).or(col("b").eq(lit(0))); | ||
| let not_extracted_short_circuit_leg = (col("a") + col("b")).eq(lit(0)); | ||
| let plan = LogicalPlanBuilder::from(table_scan.clone()) |
There was a problem hiding this comment.
I think this test covers the negative case too.
| assert_optimized_plan_eq(expected, plan, None); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Can we add a test case like the one below to check if (a or b) can be extracted as a common subexpr?
select ((a or b) or d) as f1, ((a or b) or c) as f2 from t;
There was a problem hiding this comment.
This PR doesn't extract surely evaluated children of short circuiting expressions so I kept that TODO yet ( https://github.com/apache/datafusion/pull/11197/files#diff-351499880963d6a383c92e156e75019cd9ce33107724a9635853d7d4cd1898d0R955).
I will address that in a separate follow-up PR...
There was a problem hiding this comment.
It would be nice to add the test (as a negative test perhaps)
There was a problem hiding this comment.
I've adjusted the test case to test both legs of an OR expression: 8b75d82. In this PR none of them are extraceted, but the after that follow-up PR the srurely evaluated first leg (called not_extracted_short_circuit_leg_1 now) will be extracted.
| } | ||
| let is_tree = expr.short_circuits(); | ||
| let tnr = if is_tree { | ||
| TreeNodeRecursion::Jump |
There was a problem hiding this comment.
maybe we can rename is_tree to is_short_circuits
There was a problem hiding this comment.
Thanks @peter-toth and @haohuaijin -- I plan to merge this PR tomorrow unless there are other comments or other reviewers would like more time to review
|
Thanks again everyone |
Which issue does this PR close?
Part of #11194.
Rationale for this change
Curently
CommonSubexprEliminatealways skips expressions that can short circuit, but that's not quite right as only the children of those expressions should be skipped.I.e. from the plan:
the expression
a OR bcan be extracted as common expression, but from the plan:the expression
a + bcan't be extracted.What changes are included in this PR?
Changes the rule to handle short circuiting expressions better. As
VisitRecord::JumpMarkis no longer needed this PR removes it.Are these changes tested?
Yes, added new UT.
Are there any user-facing changes?
No.