alamb commented on code in PR #3824:
URL: https://github.com/apache/arrow-datafusion/pull/3824#discussion_r996289974
##########
datafusion/optimizer/src/simplify_expressions.rs:
##########
@@ -794,13 +794,20 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a,
S> {
op: Divide,
right,
} if is_null(&right) => *right,
- // A / A --> 1 (if a is not nullable)
+ // 0 / 0 -> null
BinaryExpr {
left,
op: Divide,
right,
- } if !info.nullable(&left)? && left == right => lit(1),
-
+ } if is_zero(&left) && is_zero(&right) =>
Expr::Literal(ScalarValue::Int32(None)),
+ // A / 0 --> DivideByZeroError
+ BinaryExpr {
+ left: _,
+ op: Divide,
+ right,
+ } if is_zero(&right) => {
Review Comment:
```sql
❯ create table foo as select * from (values (1), (2), (NULL)) as sql;
❯ select * from foo;
+---------+
| column1 |
+---------+
| 1 |
| 2 |
| |
+---------+
3 rows in set. Query took 0.002 seconds.
❯ select column1 / 0 from foo;
ArrowError(DivideByZero)
```
👍
I agree on the should be checking for nullability
##########
datafusion/optimizer/src/simplify_expressions.rs:
##########
@@ -794,13 +794,20 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a,
S> {
op: Divide,
right,
} if is_null(&right) => *right,
- // A / A --> 1 (if a is not nullable)
+ // 0 / 0 -> null
Review Comment:
I think the expected behavior of a rewrite rule is the same behavior as the
current expression evaluator.
One way to check the existing behavior is to use `datafusion-cli`. In this
case the result of `0/0` is null:
```shell
$ datafusion-cli
DataFusion CLI v13.0.0
❯ select 0 / 0
;
+---------------------+
| Int64(0) / Int64(0) |
+---------------------+
| |
+---------------------+
```
##########
datafusion/optimizer/src/simplify_expressions.rs:
##########
@@ -794,13 +794,20 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a,
S> {
op: Divide,
right,
} if is_null(&right) => *right,
- // A / A --> 1 (if a is not nullable)
+ // 0 / 0 -> null
Review Comment:
BTW if the behavior (error or NULL) is inconsistent in DataFusion I think we
should file that as a separate ticket / PR rather than try to make it
consistent in this one
--
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]
