alamb commented on code in PR #25039:
URL: https://github.com/apache/datafusion/pull/25039#discussion_r4056750262
##########
datafusion/sql/src/statement.rs:
##########
@@ -224,6 +225,56 @@ fn calc_inline_constraints_from_columns(columns:
&[ColumnDef]) -> Vec<TableConst
constraints
}
+/// Rejects placeholders in a `CREATE FUNCTION` body or argument default
+/// expression that do not reference a declared argument (e.g. `$3` for a
+/// two-argument function) at definition time, instead of deferring the error
+/// to function invocation.
+fn validate_function_body_placeholders(expr: &Expr, arg_count: usize) ->
Result<()> {
Review Comment:
So in particular, I recommend creating CreateFunction::try_new or something
in the LogicalExpr that validates whatever semantic rules are.
We can do this refactor as a follow on PR too.
##########
datafusion/core/tests/user_defined/user_defined_scalar_functions.rs:
##########
@@ -1125,23 +1125,169 @@ async fn create_scalar_function_from_sql_statement()
-> Result<()> {
"#;
assert!(ctx.sql(bad_definition_sql).await.is_err());
- // FIXME: Definitions with invalid placeholders are allowed, fail at
runtime
+ // invalid placeholders are rejected at definition time
let bad_expression_sql = r#"
CREATE FUNCTION better_add(DOUBLE, DOUBLE)
RETURNS DOUBLE
RETURN $1 + $3
"#;
- assert!(ctx.sql(bad_expression_sql).await.is_ok());
+ let err = ctx
+ .sql(bad_expression_sql)
+ .await
+ .expect_err("invalid placeholder");
+ let expected = "Error during planning: Invalid placeholder, out of range:
$3";
Review Comment:
I recommend making these .slt tests so they test the end to end (and so that
they don't change if we move where this code is located)
##########
datafusion/sql/src/statement.rs:
##########
@@ -224,6 +225,56 @@ fn calc_inline_constraints_from_columns(columns:
&[ColumnDef]) -> Vec<TableConst
constraints
}
+/// Rejects placeholders in a `CREATE FUNCTION` body or argument default
+/// expression that do not reference a declared argument (e.g. `$3` for a
+/// two-argument function) at definition time, instead of deferring the error
+/// to function invocation.
+fn validate_function_body_placeholders(expr: &Expr, arg_count: usize) ->
Result<()> {
Review Comment:
I personally think this is the wrong place to be doing semantic checks (the
content of the check) like this -- Among other reasons, if we do semantic
checks here then it doesn't apply to the DataFrame API. See the description
here (https://github.com/apache/datafusion-sqlparser-rs#syntax-vs-semantics
This is kind of mentioned in the docs on planning:
https://docs.rs/datafusion/latest/datafusion/index.html#query-planning-and-execution-overview
however it isn't explicitly stated anywhere in the DataFusion docs. I will
draft a PR to do so
--
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]
