alamb commented on code in PR #14033:
URL: https://github.com/apache/datafusion/pull/14033#discussion_r1907660263
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -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(
Review Comment:
Another potential way to to keep the context would be something like
```rust
.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
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -386,13 +380,14 @@ impl CaseExpr {
let then_value = self.when_then_expr[0].1.evaluate(batch)?;
let then_value = Scalar::new(then_value.into_array(1)?);
- // keep `else_expr`'s data type and return type consistent
- let e = self.else_expr.as_ref().unwrap();
- let expr = try_cast(Arc::clone(e), &batch.schema(), return_type)
- .unwrap_or_else(|_| Arc::clone(e));
- let else_ = Scalar::new(expr.evaluate(batch)?.into_array(1)?);
-
- Ok(ColumnarValue::Array(zip(&when_value, &then_value, &else_)?))
+ if let Some(e) = self.else_expr() {
+ // keep `else_expr`'s data type and return type consistent
+ let expr = try_cast(Arc::clone(e), &batch.schema(), return_type)?;
+ let else_ = Scalar::new(expr.evaluate(batch)?.into_array(1)?);
+ Ok(ColumnarValue::Array(zip(&when_value, &then_value, &else_)?))
+ } else {
+ internal_err!("expression did not evaluate to an array")
+ }
Review Comment:
This is totally fine . Just FYI can write this with less indenting (and
returning the error first) with slightly different syntax
```suggestion
let Some(e) = self.else_expr() else {
return internal_err!("expression did not evaluate to an array");
};
// keep `else_expr`'s data type and return type consistent
let expr = try_cast(Arc::clone(e), &batch.schema(), return_type)?;
let else_ = Scalar::new(expr.evaluate(batch)?.into_array(1)?);
Ok(ColumnarValue::Array(zip(&when_value, &then_value, &else_)?))
```
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -315,10 +313,9 @@ impl CaseExpr {
remainder = and_not(&remainder, &when_value)?;
}
- if let Some(e) = &self.else_expr {
+ if let Some(e) = self.else_expr() {
// 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())?;
Review Comment:
likewise here it is a good change not to silently ignore the error -- that
would likely just make any future error more confusing
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -242,10 +244,9 @@ impl CaseExpr {
remainder = and_not(&remainder, &when_match)?;
}
- if let Some(e) = &self.else_expr {
+ if let Some(e) = self.else_expr() {
// 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())?;
Review Comment:
I think this change is an improvement -- previously the error was ignored.
In this new formulation it is returned.
--
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
