Simplify error handling in case.rs (#13990) #14033
Conversation
|
Thank you @cj-zhukov ! It seems as if there is a compile error in this PR: https://github.com/apache/datafusion/actions/runs/12646530220/job/35237388507?pr=14033 |
|
@alamb Andrew, I noticed the build fails unless I import |
There was a problem hiding this comment.
Thanks @cj-zhukov -- this looks quite nice to me ❤️
@alamb Andrew, I noticed the build fails unless I import
datafusion_common::DataFusionError, which is not used in my PR changes but appears necessary for compatibility with the main branch. Please let me know if there's a better way to handle this.
I am not sure why this was needed -- DataFusionError is used indirectly by the macros you use (e.g. internal_datafusion_err uses DataFusionError internally. Maybe it has something to do with scoping rules 🤔 I am not sure
| // keep `else_expr`'s data type and return type consistent | ||
| let expr = try_cast(Arc::clone(e), &batch.schema(), return_type.clone()) | ||
| .unwrap_or_else(|_| Arc::clone(e)); | ||
| let expr = try_cast(Arc::clone(e), &batch.schema(), return_type.clone())?; |
There was a problem hiding this comment.
I think this change is an improvement -- previously the error was ignored. In this new formulation it is returned.
| // keep `else_expr`'s data type and return type consistent | ||
| let expr = try_cast(Arc::clone(e), &batch.schema(), return_type.clone()) | ||
| .unwrap_or_else(|_| Arc::clone(e)); | ||
| let expr = try_cast(Arc::clone(e), &batch.schema(), return_type.clone())?; |
There was a problem hiding this comment.
likewise here it is a good change not to silently ignore the error -- that would likely just make any future error more confusing
| @@ -369,11 +366,8 @@ impl CaseExpr { | |||
| // evaluate when expression | |||
| let when_value = self.when_then_expr[0].0.evaluate(batch)?; | |||
| let when_value = when_value.into_array(batch.num_rows())?; | |||
| let when_value = as_boolean_array(&when_value).map_err(|e| { | |||
| DataFusionError::Context( | |||
There was a problem hiding this comment.
Another potential way to to keep the context would be something like
.map_err(|e| e.context("WHEN expression did not return a BooleanArray"))I think what you have here is good though as it makes it clear this is an internal / non expected error
Co-authored-by: Andrew Lamb <[email protected]>
|
@alamb Andrew, thank you for feedback and useful advices. Really appreciate your work that helps me improve my Rust code. |
|
Thanks again @cj-zhukov |
Which issue does this PR close?
Closes #13990.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?