kosiew commented on code in PR #23063:
URL: https://github.com/apache/datafusion/pull/23063#discussion_r3755227859
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -362,10 +382,19 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
.map(|arg| arg.to_string())
.collect::<Vec<_>>()
.join(",");
- let verbose_alias = format!("{name}({arg_names})");
-
- return Ok(Expr::ScalarFunction(inner).alias(verbose_alias));
- }
+ Some(format!("{name}({arg_names})"))
+ };
+ let expr = Expr::ScalarFunction(inner);
+ let span = if self.options.collect_spans {
+ Span::try_from_sqlparser_span(sql_parser_span)
+ } else {
+ None
+ };
+ validate_function_expr(&expr, schema, &name, span)?;
Review Comment:
I think this validation is happening too early. `validate_function_expr`
calls `Expr::to_field` while we are still in `SqlToRel`, before the
`TypeCoercion` analyzer has inserted any necessary casts.
That changes behavior for valid coercible calls. I reproduced it with the
existing `select_where_nullif_division` test: `nullif(c4 + c5, 0)` reaches this
point as `nullif(Int32, Int64)` and is rejected, even though the analyzer
normally coerces the literal and the query should plan successfully.
Could we avoid validating here and keep the existing post-coercion
validation boundary? I would prefer attaching the best available argument or
subexpression span when `verify_function_arguments` emits the diagnostic, even
if that span is approximate, rather than changing when validation happens.
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -81,6 +81,27 @@ fn find_closest_match(candidates: Vec<String>, target: &str)
-> Option<String> {
})
}
+fn validate_function_expr(
+ expr: &Expr,
+ schema: &DFSchema,
+ function_name: &str,
+ function_span: Option<Span>,
+) -> Result<()> {
+ expr.to_field(schema).map(|_| ()).map_err(|err| {
+ let expected_message = format!("invalid argument type(s) for
'{function_name}'");
Review Comment:
I think aliases will miss the call-site span here. `function_name` is the
spelling from the SQL query, but `verify_function_arguments` builds the
diagnostic using the resolved UDF's canonical name.
For example, `pow(first_name, first_name)` resolves to `power`, so the
diagnostic says `invalid argument type(s) for 'power'`, while this helper looks
for `invalid argument type(s) for 'pow'`. The messages do not match, so the
original diagnostic is returned without the span.
If this matching approach remains, could we compare against the canonical
resolved function name while still attaching the span from the SQL call? It
would also be good to add alias regression coverage for the applicable scalar,
aggregate, and window paths.
--
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]
