alamb commented on code in PR #17450:
URL: https://github.com/apache/datafusion/pull/17450#discussion_r2326050206


##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1399,6 +1399,18 @@ impl<S: SimplifyInfo> TreeNodeRewriter for 
Simplifier<'_, S> {
             // Rules for Case
             //
 
+            // CASE WHEN true THEN A ... END --> A
+            Expr::Case(Case {
+                expr: None,
+                when_then_expr,
+                else_expr: _,
+            }) if !when_then_expr.is_empty()
+                && matches!(when_then_expr[0].0.as_ref(), 
+                    Expr::Literal(ScalarValue::Boolean(Some(true)), _)) =>

Review Comment:
   
   can we please use `is_true` to follow the pattern in the rest of this method?
   
   
https://github.com/apache/datafusion/blob/4b9a468cc1949062cf3cd8685ba8ced377fd212e/datafusion/optimizer/src/simplify_expressions/utils.rs#L193-L198



##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -3552,6 +3564,61 @@ mod tests {
         );
     }
 
+    #[test]
+    fn simplify_expr_case_when_true() {
+        // CASE WHEN true THEN 1 ELSE x END --> 1

Review Comment:
   can we please add some negative tests too -- like `CASE WHEN  a THEN 1 ELSE 
2 END`?



##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1399,6 +1399,18 @@ impl<S: SimplifyInfo> TreeNodeRewriter for 
Simplifier<'_, S> {
             // Rules for Case
             //
 
+            // CASE WHEN true THEN A ... END --> A
+            Expr::Case(Case {
+                expr: None,
+                when_then_expr,
+                else_expr: _,
+            }) if !when_then_expr.is_empty()
+                && matches!(when_then_expr[0].0.as_ref(), 
+                    Expr::Literal(ScalarValue::Boolean(Some(true)), _)) =>
+            {
+                Transformed::yes((*when_then_expr[0].1).clone())

Review Comment:
   I think it would be good to avoid this clone too, 
   
   How about something like this:
   
   ```suggestion
                   let (when, then_) = when_then_expr.swap_remove(0);
                   Transformed::yes(*when)
   ```



-- 
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: github-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to