alamb commented on code in PR #8928:
URL: https://github.com/apache/arrow-datafusion/pull/8928#discussion_r1460918851
##########
datafusion/sqllogictest/test_files/select.slt:
##########
@@ -1129,5 +1129,35 @@ FROM t AS A, (SELECT * FROM t WHERE x = 0) AS B;
0 0
0 0
+query TT
+explain select coalesce(1, y/x), coalesce(2, y/x) from t;
Review Comment:
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
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -655,6 +658,20 @@ impl TreeNodeVisitor for ExprIdentifierVisitor<'_> {
}
}
+/// Check if the expression is short-circuit expression
+fn is_short_circuit_expression(expr: &Expr) -> bool {
Review Comment:
I think we should put this method on expr so it is easier to find and
potentially use
```rust
impl Expr {
...
/// returns true if some of this `exprs` subexpressions may not be
evaluated
/// and thus any side effects (like divide by zero) may not be encountered
fn short_circuits(&self) -> bool {
...
}
```
--
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]
