haohuaijin commented on code in PR #8957:
URL: https://github.com/apache/arrow-datafusion/pull/8957#discussion_r1462729376
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -796,18 +801,6 @@ impl<'a, S: SimplifyInfo> TreeNodeRewriter for
Simplifier<'a, S> {
op: Divide,
right,
}) if is_null(&right) => *right,
- // A / 0 -> Divide by zero error if A is not null and not floating
- // (float / 0 -> inf | -inf | NAN)
- Expr::BinaryExpr(BinaryExpr {
Review Comment:
Remove this because `A / 0` can occur in short-circuit expression. And we
can throw such an error in runtime.
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -148,6 +148,11 @@ impl CaseExpr {
// Make sure we only consider rows that have not been matched yet
let when_match = and(&when_match, &remainder)?;
+ // When no rows available for when clause, skip then clause
+ if when_match.true_count() == 0 {
+ continue;
+ }
+
Review Comment:
Because even though `when_match` is all false, the `evaluate_selection`
still evaluates the expression one time.
https://github.com/apache/arrow-datafusion/blob/827668ab3d4659329ff30e6672f398c934b803be/datafusion/physical-expr/src/expressions/case.rs#L148-L153
so the query like below, will get divide by zero error.
```
select case 1 when 2 the 4/0 end;
```
------
The root cause is that even though `BinaryExpr` receives a `RecordBatch`
with 0 rows, it also evaluates once. And, it only appears in `scalar op scalar`.
https://github.com/apache/arrow-datafusion/blob/827668ab3d4659329ff30e6672f398c934b803be/datafusion/physical-expr/src/expressions/binary.rs#L259-L268
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -281,7 +280,13 @@ impl<'a> TreeNodeRewriter for ConstEvaluator<'a> {
fn mutate(&mut self, expr: Expr) -> Result<Expr> {
match self.can_evaluate.pop() {
- Some(true) => Ok(Expr::Literal(self.evaluate_to_scalar(expr)?)),
+ Some(true) => {
+ let lit = self.evaluate_to_scalar(expr.clone());
+ match lit {
+ Ok(lit) => Ok(Expr::Literal(lit)),
+ Err(_) => Ok(expr),
+ }
+ }
Review Comment:
This is the main change
--
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]
