raghavyadav01 commented on code in PR #26548:
URL: https://github.com/apache/datafusion/pull/26548#discussion_r4343422344
##########
datafusion/optimizer/src/eliminate_filter.rs:
##########
@@ -113,10 +114,54 @@ 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) if in_list.list.iter().any(is_null_literal) => {
Review Comment:
Done in b20f1f8 — the `simplify_filter_predicate` doc comment now points at
the IN list case, and `simplify_filter_in_list` carries the reasoning and the
reason it can't live in `ExprSimplifier`.
##########
datafusion/optimizer/src/eliminate_filter.rs:
##########
@@ -113,10 +114,54 @@ 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) if in_list.list.iter().any(is_null_literal) => {
Review Comment:
Good call — done. It now always attempts the rewrite and reports whether
anything changed by comparing the list length before and after, which dropped
the guard and collapsed the two "never TRUE" branches into one.
##########
datafusion/optimizer/src/eliminate_filter.rs:
##########
@@ -495,4 +540,85 @@ mod tests {
EmptyRelation: rows=0
")
}
+
+ #[test]
Review Comment:
Done — unit tests removed and replaced with end-to-end tests in
`in_list.slt`, covering the plans, the rows returned, and that `SELECT 3 IN (1,
NULL)` is still NULL outside a filter.
Worth flagging what the slt tests showed, since it narrows the change: at
SQL level only the NULL-dropping case actually differs. `x IN (NULL)` and `x
NOT IN (..., NULL)` already reach EmptyRelation on main via existing
simplification, so those tests are regression guards rather than new behaviour.
I verified by reverting `eliminate_filter.rs` to origin/main — exactly one slt
mismatch, the dropped NULL — and zero with the change.
I also kept the lists above THRESHOLD_INLINE_INLIST so they stay IN lists;
below it they get rewritten into OR chains and the plan no longer shows what
the test is about.
--
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]
