alamb commented on a change in pull request #1257:
URL: https://github.com/apache/arrow-datafusion/pull/1257#discussion_r744659045
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -453,7 +450,7 @@ mod tests {
fn test_simplify_and_and_false() -> Result<()> {
let expr =
binary_expr(lit(ScalarValue::Boolean(None)), Operator::And,
lit(false));
- let expr_eq = lit(false);
+ let expr_eq = null;
Review comment:
I think the original answer of `false` is correct. Here is the result in
postgres:
```
alamb=# select null and false;
?column?
----------
f
(1 row)
alamb=#
```
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -100,6 +100,8 @@ fn is_false(expr: &Expr) -> bool {
fn simplify(expr: &Expr) -> Expr {
match expr {
+ Expr::BinaryExpr { left, .. } if is_literal_null(left) =>
*left.clone(),
Review comment:
Thank you @Jimexist
I don't think this is true for `Operator::IsDistinctFrom` and
`Operator::IsNotDistinctFrom` , `Operator::And` and `Operator::Or`, so I
think we should special case those Operators
Or perhaps to be more conservative make a whitelist of the arithmetic
operators.
https://github.com/apache/arrow-datafusion/blob/81fae230b81b97e93bbb95b284cfda6f4d59552e/datafusion/src/logical_plan/operators.rs#L56-L58
examples
`null IS DISTINCT FROM null` --> `false` (but this PR would rewrite to
`null` I think)
`true OR null` --> `true` (but this PR would rewrite it to `null` I think)
--
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]
