alamb commented on a change in pull request #9682:
URL: https://github.com/apache/arrow/pull/9682#discussion_r602597169
##########
File path: rust/datafusion/src/logical_plan/expr.rs
##########
@@ -149,6 +149,13 @@ pub enum Expr {
/// The `DataType` the expression will yield
data_type: DataType,
},
+ /// Casts the expression to a given type and returns null if not possible.
This expression is guaranteed to have a fixed type.
Review comment:
it may be good to add a note to `Cast` saying that a runtime error will
result if the expression can not be cast.
##########
File path: rust/datafusion/src/logical_plan/expr.rs
##########
@@ -303,6 +311,7 @@ impl Expr {
}
}
Expr::Cast { expr, .. } => expr.nullable(input_schema),
+ Expr::TryCast { expr, .. } => expr.nullable(input_schema),
Review comment:
It seems like given the semantics of `TryCast` that this should always
return `true`
##########
File path: rust/datafusion/tests/sql.rs
##########
@@ -2528,7 +2528,7 @@ async fn in_list_array() -> Result<()> {
,c1 IN ('x', 'y') AS utf8_in_false
,c1 NOT IN ('x', 'y') AS utf8_not_in_true
,c1 NOT IN ('a', 'c') AS utf8_not_in_false
- ,CAST(CAST(c1 AS int) AS varchar) IN ('a', 'c') AS utf8_in_null
Review comment:
I don't really understand what the original test was testing 🤔 but the
change looks better
##########
File path: rust/datafusion/src/physical_plan/planner.rs
##########
@@ -645,10 +653,11 @@ impl DefaultPhysicalPlanner {
&list_expr_data_type,
&value_expr_data_type,
) {
- expressions::cast(
+ expressions::cast_with_options(
Review comment:
Doesn't `expressions::cast` do the same thing? I wonder if we need to
change this?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
