Jefffrey commented on code in PR #18019:
URL: https://github.com/apache/datafusion/pull/18019#discussion_r2453947233
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -668,8 +703,44 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
qualifier: qualifier.into(),
options: Box::new(WildcardOptions::default()),
};
-
- Ok(expr)
+ Ok((expr, None))
+ }
+ // PostgreSQL dialect uses ExprNamed variant with expression for
name
+ FunctionArg::ExprNamed {
+ name,
+ arg: FunctionArgExpr::Expr(arg),
+ operator: _,
+ } => {
+ let expr = self.sql_expr_to_logical_expr(arg, schema,
planner_context)?;
+ let arg_name = match name {
+ SQLExpr::Identifier(ident) =>
crate::utils::normalize_ident(ident),
+ _ => {
+ return plan_err!(
+ "Named argument must use a simple identifier, got:
{name:?}"
+ )
+ }
+ };
+ Ok((expr, Some(arg_name)))
+ }
Review Comment:
```suggestion
FunctionArg::ExprNamed {
name: SQLExpr::Identifier(name),
arg: FunctionArgExpr::Expr(arg),
operator: _,
} => {
let expr = self.sql_expr_to_logical_expr(arg, schema,
planner_context)?;
let arg_name = crate::utils::normalize_ident(name);
Ok((expr, Some(arg_name)))
}
```
Thoughts on simplifying to something like this? (same for below arm)
I can't visualize the case this is guarding against so I don't know if its
better to trade off more concise code with a less specific error (since we'd
then hit the `not_impl_err!("Unsupported qualified wildcard argument:
{sql:?}")` catchall now) if the case is rare, or if it is indeed better to keep
current version and have a more specific error 🤔
--
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]
