-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: Simplify CASE WHEN true THEN expr to expr #17450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Simplify CASE WHEN true THEN expr to expr #17450
Conversation
Add optimization rule to simplify CASE expressions where the first condition is always true (literal true), reducing them to just the THEN expression. This eliminates unnecessary branching and casting for cases like "CASE WHEN true THEN 1 ELSE x END" which now simplifies to "1". Fixes apache#17448
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the quick turnaround @EeshanBembi
I left some comments / suggestions
@@ -3552,6 +3564,61 @@ mod tests { | |||
); | |||
} | |||
|
|||
#[test] | |||
fn simplify_expr_case_when_true() { | |||
// CASE WHEN true THEN 1 ELSE x END --> 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we please add some negative tests too -- like CASE WHEN a THEN 1 ELSE 2 END
?
else_expr: _, | ||
}) if !when_then_expr.is_empty() | ||
&& matches!(when_then_expr[0].0.as_ref(), | ||
Expr::Literal(ScalarValue::Boolean(Some(true)), _)) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we please use is_true
to follow the pattern in the rest of this method?
datafusion/datafusion/optimizer/src/simplify_expressions/utils.rs
Lines 193 to 198 in 4b9a468
pub fn is_true(expr: &Expr) -> bool { | |
match expr { | |
Expr::Literal(ScalarValue::Boolean(Some(v)), _) => *v, | |
_ => false, | |
} | |
} |
&& matches!(when_then_expr[0].0.as_ref(), | ||
Expr::Literal(ScalarValue::Boolean(Some(true)), _)) => | ||
{ | ||
Transformed::yes((*when_then_expr[0].1).clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to avoid this clone too,
How about something like this:
Transformed::yes((*when_then_expr[0].1).clone()) | |
let (when, then_) = when_then_expr.swap_remove(0); | |
Transformed::yes(*when) |
Summary
Adds an optimization rule to simplify CASE expressions where the first condition is always true (literal
true
), reducing them to just the THEN expression.This fixes the issue where expressions like:
CASE WHEN true THEN 1 ELSE x END
were not being simplified to just
1
.Changes
expr_simplifier.rs
that detectsCASE WHEN true THEN expr
patternsTest Plan
Before/After
Before:
After:
Fixes #17448