alamb commented on code in PR #26548:
URL: https://github.com/apache/datafusion/pull/26548#discussion_r4372523580
##########
datafusion/optimizer/src/eliminate_filter.rs:
##########
@@ -113,10 +118,62 @@ 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,
+ mut list,
+ negated,
+ } = in_list;
+
+ // `retain` drops the `NULL`s in place. Most lists have none, and that case
+ // then costs one pass and no allocation.
+ let len_before = list.len();
+ list.retain(|e| !is_null_literal(e));
+
+ if list.len() == len_before {
Review Comment:
this is nice
##########
datafusion/optimizer/src/eliminate_filter.rs:
##########
@@ -113,10 +118,62 @@ 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.
Review Comment:
This comment states the same thing several times. I think this would be
adequate:
```suggestion
/// `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 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]
