[PERF] Evaluate only true/false side of if_else if predicate is boolean #2222
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2222 +/- ##
=======================================
Coverage ? 85.59%
=======================================
Files ? 71
Lines ? 7586
Branches ? 0
=======================================
Hits ? 6493
Misses ? 1093
Partials ? 0 |
src/daft-dsl/src/expr.rs
Outdated
| Err(_) => Err(DaftError::TypeError(format!("Expected if_true and if_false arguments for if_else to be castable to the same supertype, but received {if_true_field} and {if_false_field}"))) | ||
| match predicate.as_ref() { | ||
| Expr::Literal(lit::LiteralValue::Boolean(true)) => if_true.to_field(schema), | ||
| Expr::Literal(lit::LiteralValue::Boolean(false)) => if_false.to_field(schema), |
There was a problem hiding this comment.
The tricky bit for me in this PR is the conditional selection of the resultant column's name... Which feels a little ick.
Though I'm not sure what the best behavior is here. It feels a little icky to me though that depending on the inputs, the output expression name can change.
There was a problem hiding this comment.
Agreed, I'll keep it consistent with the current behavior to use the if_true expression name.
Though I think this may not be the best user experience either, in fact I'm not sure if there is a way to aptly name an if_else expression. We can consider writing in the if_else docs that it's best practice to use if_else in a with_column, or with an alias
Closes #2190
If the predicate for an if_else statement is a boolean, i.e true/false, we only need to evaluate one side of the expression.