jcsherin commented on code in PR #12811:
URL: https://github.com/apache/datafusion/pull/12811#discussion_r1800655570
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -217,6 +217,41 @@ fn get_casted_value(
}
}
+fn rewrite_null_expr_and_data_type(
+ args: &[Arc<dyn PhysicalExpr>],
+ expr_type: &DataType,
+) -> (Arc<dyn PhysicalExpr>, DataType) {
+ let expr = Arc::clone(&args[0]);
+
+ // The input expression and the return is type is unchanged
+ // when the input expression is not NULL.
+ if !expr_type.is_null() {
+ return (expr, expr_type.clone());
+ }
+
+ // Rewrites the NULL expression (1st argument) with an expression
+ // which is the same data type as the default value (3rd argument).
+ // Also rewrites the return type with the same data type as the
+ // default value.
+ //
+ // If a default value is not provided, or it is NULL the original
+ // expression (1st argument) and return type is returned without
+ // any modifications.
+ get_scalar_value_from_args(args, 2)
+ .unwrap()
Review Comment:
Sorry, I should have avoided the use of `unwrap()` here which will panic 😅.
Instead we should propagate the error up the call stack.
```suggestion
get_scalar_value_from_args(args, 2)?
```
Inside `get_scalar_value_from_args` an error is returned if a `Literal` is
not found at the specified index,
https://github.com/apache/datafusion/blob/f9e8e07e8a3af4f1f4836259c6f87636bc1b9631/datafusion/physical-plan/src/windows/mod.rs#L173-L176
For that we'll need to change the function signature to return a
`Result<(Arc<dyn PhysicalExpr>, DataType)>` type.
```rust
fn rewrite_null_expr_and_data_type(
args: &[Arc<dyn PhysicalExpr>],
expr_type: &DataType,
) -> Result<(Arc<dyn PhysicalExpr>, DataType)> // <- wrapped in `Result()`
```
And the callers changed to do an early return in case of any errors.
```rust
// rewrite NULL expression and the return datatype
let (arg, out_data_type) =
rewrite_null_expr_and_data_type(args, out_data_type)?; // <-
adds `?`
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
