kosiew commented on code in PR #25143:
URL: https://github.com/apache/datafusion/pull/25143#discussion_r3988603167
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -2432,6 +2436,26 @@ fn simplify_inlist_set_operation(
}))
}
+/// Conservatively checks the inputs whose evaluation can change when lowering
+/// CASE to AND/OR. [`Expr`] has no general fallibility analysis: only columns
and
+/// literals are admitted from conditional branches, including later WHEN
conditions.
+/// The first WHEN already runs on every row, but must not be volatile because
+/// the rewrite can evaluate it more than once.
+fn can_lower_case_to_boolean(
+ when_then_expr: &[(Box<Expr>, Box<Expr>)],
+ else_expr: Option<&Expr>,
+) -> bool {
+ let is_leaf = |expr: &Expr| matches!(expr, Expr::Column(_) |
Expr::Literal(..));
+ when_then_expr.iter().enumerate().all(|(i, (when, then))| {
+ is_leaf(then)
+ && if i == 0 {
+ !when.is_volatile()
Review Comment:
I think there is still a correctness issue with allowing the first `WHEN` as
long as it is non-volatile. It can still fail, and the Boolean rewrite can
eliminate its evaluation entirely.
For example, `SELECT CASE WHEN CAST(s AS INT) > 0 THEN false ELSE false END
FROM (VALUES ('abc')) t(s)` should raise the cast error because CASE evaluates
its first WHEN for every row. With this rewrite, both branches can simplify to
`false`, so the condition is never evaluated and the query returns `false`
instead.
Could we require the first `WHEN` to be a column or literal as well, unless
we add a sound fallibility analysis? It would also be good to add this example
as a regression test, particularly with identical literal results so we cover
the case where simplification removes the condition.
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -2432,6 +2436,26 @@ fn simplify_inlist_set_operation(
}))
}
+/// Conservatively checks the inputs whose evaluation can change when lowering
+/// CASE to AND/OR. [`Expr`] has no general fallibility analysis: only columns
and
+/// literals are admitted from conditional branches, including later WHEN
conditions.
Review Comment:
Once the first-WHEN guard is tightened, could we update this comment to
match the invariant and say that all `WHEN` conditions and branch outputs must
be columns or literals? I think spelling that out here will make it less likely
that a future special case accidentally reintroduces this error-preservation
gap.
--
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]
