alamb commented on code in PR #4090:
URL: https://github.com/apache/arrow-datafusion/pull/4090#discussion_r1013288612
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -365,6 +367,32 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a,
S> {
None => lit_bool_null(),
}
}
+ Expr::InList {
+ expr: _expr,
+ list,
+ negated,
+ } if list.is_empty() => lit(negated),
Review Comment:
I don't know how important this is, but this only holds if `expr` is non
null 🤔
```
postgres=# select null in (1);
?column?
----------
(1 row)
```
However, note that you can't even make an empty inlist in postgres so I
don't know how much this edge case matters
```sql
postgres=# select null in ();
ERROR: syntax error at or near ")"
LINE 1: select null in ();
```
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -365,6 +367,32 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a,
S> {
None => lit_bool_null(),
}
}
+ Expr::InList {
Review Comment:
```suggestion
// expr IN () --> false
// expr NOT IN () --> true
Expr::InList {
```
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -365,6 +367,32 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a,
S> {
None => lit_bool_null(),
}
}
+ Expr::InList {
+ expr: _expr,
+ list,
+ negated,
+ } if list.is_empty() => lit(negated),
+
+ Expr::InList {
Review Comment:
```suggestion
// if expr is a single column reference:
// expr IN (A, B, ...) --> (expr = A) OR (expr = B) OR (expr = C)
Expr::InList {
```
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -365,6 +367,32 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a,
S> {
None => lit_bool_null(),
}
}
+ Expr::InList {
+ expr: _expr,
+ list,
+ negated,
+ } if list.is_empty() => lit(negated),
+
+ Expr::InList {
+ expr,
+ list,
+ negated,
+ } if list.len() <= THRESHOLD_INLINE_INLIST &&
expr.try_into_col().is_ok() => {
Review Comment:
I recommend also adding an explicit case for `list.len() == 1` (without
`expr.try_into_col()`)
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -365,6 +367,32 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a,
S> {
None => lit_bool_null(),
}
}
+ Expr::InList {
+ expr: _expr,
+ list,
+ negated,
+ } if list.is_empty() => lit(negated),
+
+ Expr::InList {
+ expr,
+ list,
+ negated,
+ } if list.len() <= THRESHOLD_INLINE_INLIST &&
expr.try_into_col().is_ok() => {
+ let first_val = list[0].clone();
+ if negated {
+ list.into_iter()
+ .skip(1)
+ .fold((*expr.clone()).not_eq(first_val), |acc, y| {
+ (*expr.clone()).not_eq(y.clone()).and(acc)
+ })
+ } else {
+ list.into_iter()
+ .skip(1)
+ .fold((*expr.clone()).eq(first_val), |acc, y| {
+ (*expr.clone()).eq(y.clone()).or(acc)
+ })
+ }
Review Comment:
I think you can simplify this code using `reduce`:
```suggestion
if negated {
list.into_iter()
.reduce(|acc, y| {
(*expr.clone()).not_eq(y.clone()).and(acc)
}).unwrap()
} else {
list.into_iter()
.reduce(|acc, y| {
(*expr.clone()).eq(y.clone()).or(acc)
}).unwrap()
}
```
--
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]
