perf: Simplify CASE for any WHEN TRUE #17602
Conversation
petern48
changed the title
| // if let guard is not stabilized so we can't use it yet: https://github.com/rust-lang/rust/issues/51114 | ||
| // Once it's supported we can avoid searching through when_then_expr twice in the below .any() and .position() calls | ||
| // }) if let Some(i) = when_then_expr.iter().position(|(when, _)| is_true(when.as_ref())) => { | ||
| }) if when_then_expr | ||
| .iter() | ||
| .any(|(when, _)| is_true(when.as_ref())) => | ||
| { | ||
| if let Some(i) = when_then_expr | ||
| .iter() | ||
| .position(|(when, _)| is_true(when.as_ref())) | ||
| { |
There was a problem hiding this comment.
Removing the condition guard results in making the case rule below this one inaccessible, so we can't just remove the .any() condition here. An alternative approach would be to move all of this logic into the same block as the rule below this, but the code wouldn't be so clean. I feel like iterating over the when_then_expr vector twice for now isn't too bad since these shouldn't really get too long, but I'm happy to hear other thoughts.
There was a problem hiding this comment.
Makes sense to me. Can also simplify this to:
let i = when_then_expr
.iter()
.position(|(when, _)| is_true(when.as_ref()))
.unwrap();Since the guard guarantees the unwrap is safe.
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
Outdated
Show resolved
Hide resolved
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
Looks good to me; don't know when if let guards will be stabilized but not sure if there is a cleaner way to achieve it otherwise 🤔
| // if let guard is not stabilized so we can't use it yet: https://github.com/rust-lang/rust/issues/51114 | ||
| // Once it's supported we can avoid searching through when_then_expr twice in the below .any() and .position() calls | ||
| // }) if let Some(i) = when_then_expr.iter().position(|(when, _)| is_true(when.as_ref())) => { | ||
| }) if when_then_expr | ||
| .iter() | ||
| .any(|(when, _)| is_true(when.as_ref())) => | ||
| { | ||
| if let Some(i) = when_then_expr | ||
| .iter() | ||
| .position(|(when, _)| is_true(when.as_ref())) | ||
| { |
There was a problem hiding this comment.
Makes sense to me. Can also simplify this to:
let i = when_then_expr
.iter()
.position(|(when, _)| is_true(when.as_ref()))
.unwrap();Since the guard guarantees the unwrap is safe.
4 participants
Which issue does this PR close?
Rationale for this change
Improves perf
What changes are included in this PR?
Extends the case simplification by simplifying the expr when any of the when statements are true. Previously the simplification only applied to when the first element was true.
Are these changes tested?
Yes
Are there any user-facing changes?
No