alamb commented on code in PR #26548:
URL: https://github.com/apache/datafusion/pull/26548#discussion_r4361344186
##########
datafusion/optimizer/src/eliminate_filter.rs:
##########
@@ -113,10 +118,60 @@ fn simplify_filter_predicate(expr: Expr) ->
SimplifiedFilterPredicate {
Expr::BinaryExpr(binary) if binary.op == Operator::Or => {
simplify_filter_or(*binary.left, *binary.right)
}
+ Expr::InList(in_list) => simplify_filter_in_list(in_list),
expr => simplified(FilterPredicate::Expression(expr), false),
}
}
+fn is_null_literal(expr: &Expr) -> bool {
+ matches!(expr, Expr::Literal(scalar, _) if scalar.is_null())
+}
+
+/// Simplifies an `IN` list under filter semantics by dropping `NULL` members.
+///
+/// `x IN (a, NULL)` is `TRUE` exactly when `x` matches `a`: a `NULL` member
can
+/// only turn `FALSE` into `NULL`, and `NULL` rejects a row just like `FALSE`.
+/// The `NULL` members can therefore be dropped, and a list of nothing but
+/// `NULL`s rejects every row.
+///
+/// `x NOT IN (a, NULL)` is `FALSE` when `x` matches `a` and `NULL` otherwise,
so
+/// it is never `TRUE` and rejects every row.
+///
+/// This is only sound under filter semantics; the same rewrite in a general
+/// expression context would change `SELECT 3 IN (1, NULL)` from `NULL` to
`FALSE`.
+fn simplify_filter_in_list(in_list: InList) -> SimplifiedFilterPredicate {
+ let InList {
+ expr,
+ list,
+ negated,
+ } = in_list;
+
+ let len_before = list.len();
+ let list: Vec<Expr> = list.into_iter().filter(|e|
!is_null_literal(e)).collect();
Review Comment:
this makes a (deep) copy of each expression and I expect the common case
will be that no entry is null -- can you please avoid making the copy before
copying?
And then we can perhaps use Vec::retain to modify the existing vec rather
than make a new copy
##########
datafusion/sqllogictest/test_files/in_list.slt:
##########
@@ -1072,3 +1072,91 @@ ORDER BY n
# Cleanup
statement ok
DROP TABLE in_list_case_items;
+
Review Comment:
Do you know why the plan that changed in
https://github.com/apache/datafusion/pull/25187/changes#r4330163354 is not also
changed in this PR?
I would exepect to see that plan simplified without the NULL in the list
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
