Re: [PR] fix(expr): coerce literal arguments in return_field_from_args for UDFs [datafusion]

["Jefffrey (via GitHub)" <gi...@apache.org>]

Jefffrey commented on code in PR #20012:
URL: https://github.com/apache/datafusion/pull/20012#discussion_r2754663178


##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -598,13 +598,32 @@ impl ExprSchemable for Expr {
                         )
                     })?;
 
-                let arguments = args
+                let coerced_values: Vec<Option<ScalarValue>> = args
                     .iter()
-                    .map(|e| match e {
-                        Expr::Literal(sv, _) => Some(sv),
-                        _ => None,
+                    .zip(new_fields.iter())
+                    .map(|(expr, field)| {
+                        let mut current_expr = expr;
+
+                        // Loop to remove all casting layers
+                        loop {
+                            match current_expr {
+                                Expr::Cast(cast) => current_expr = &cast.expr,
+                                Expr::TryCast(cast) => current_expr = &cast.expr,
+                                _ => break,
+                            }
+                        }
+
+                        let literal = if let Expr::Literal(sv, _) = current_expr {
+                            Some(sv)
+                        } else {
+                            None
+                        };
+
+                        literal.map(|sv| sv.cast_to(field.data_type())).transpose()

Review Comment:
   Ah you're right; and it highlights another current inconsistency; when `return_field_from_args()` is called in the context of `select_to_plan` (aka initially forming the LogicalPlan) it will have `Expr::Literal` passed in for scalar arguments to compute the field. However, later in the type coercion pass it adds a `Cast` wrapping this and in the current code on main this would actually mean we pass in `None` for the scalar argument since we don't unwrap the cast as this PR does, potentially leading to an inconsistent result 🤔
   
   https://github.com/apache/datafusion/blob/66ee0afcabfd59323eeb7b5f76f60a80881bbba3/datafusion/expr/src/expr_schema.rs#L538-L548
   
   For now I don't think this is an actual bug that will crop up (we don't have too many UDFs relying on `scalar_arguments` yet) but it is definitely an inconsistency to address. It partially relates to #13825 as well since if we are able to compute these properties once upfront then it might be easier to handle.
   
   I'll try think about it more, but I suppose this is the easier fix for now; other ways to fix would be more involved such as:
   
   - #13825 way in reducing the amount of calls to `return_field_from_args()` to just once upfront; this does mean we wouldn't need this unwrapping logic so calls like `SELECT test_udf(CAST(1 AS INT))` wouldn't work as the argument is no longer a literal, it is a cast expression (I think this is intended behaviour)
   - Alter type coercion logic to immediately apply cast to literals, instead of wrapping in a `Expr::Cast`; similar to how we unwrap the casts here then apply the cast, but instead of unwrapping here we rely on type coercion to do the cast for us
   
   (I'm not advocating for the above steps, just laying out some of my thoughts as I think through 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.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org