quwin opened a new pull request, #25039:
URL: https://github.com/apache/datafusion/pull/25039
## Which issue does this PR close?
- Closes #25038
## Rationale for this change
A `CREATE FUNCTION` with a SQL body is accepted even when its `RETURN`
expression references a positional placeholder that does not match a declared
argument:
```sql
CREATE FUNCTION better_add(DOUBLE, DOUBLE)
RETURNS DOUBLE
RETURN $1 + $3 -- only two arguments are declared
```
The invalid definition is registered successfully, and the error only
surfaces when the function is invoked:
```
Optimizer rule 'simplify_expressions' failed
caused by
Execution error: Invalid placeholder, out of range: $3
```
A definition error like this should be reported at `CREATE FUNCTION` time,
not deferred to every invocation. This is acknowledged in code by two FIXMEs
(see #25038):
- `datafusion/sql/src/expr/value.rs` — "In the CREATE FUNCTION branch,
param_type = None should raise an error"
- `datafusion/core/tests/user_defined/user_defined_scalar_functions.rs` —
"Definitions with invalid placeholders are allowed, fail at runtime"
## What changes are included in this PR?
- Validate placeholders at planning time in the `Statement::CreateFunction`
arm of the SQL planner (`datafusion/sql/src/statement.rs`): after the `RETURN`
body is planned, walk it for `Expr::Placeholder` nodes and reject any
positional `$N` outside `1..=declared_arg_count` with `Invalid placeholder, out
of range: $N`, and any named placeholder (which can only survive parsing when
zero arguments were declared) with `Unknown placeholder: $N`.
- The check uses the **declared** argument count, so functions with
defaulted arguments remain callable with fewer arguments than declared.
- Doing this in the planner (rather than in `value.rs` or in the factories)
means it applies to every `FunctionFactory` implementation, and it cannot
change `PREPARE` semantics, where an empty/unknown parameter list must stay
permissive for deferred type inference. Runtime guards in the factories remain
as a defensive backstop.
- Replaced the now-superseded FIXME comment in
`datafusion/sql/src/expr/value.rs`.
## What is the testing strategy for this PR?
- Flipped the FIXME regression test in
`create_scalar_function_from_sql_statement()`: the invalid `CREATE FUNCTION`
must now fail with `Error during planning: Invalid placeholder, out of range:
$3`.
- Added `create_scalar_function_from_sql_statement_invalid_placeholders()`
covering: out-of-range positional with positional declared args; out-of-range
positional with named declared args (`RETURN $a + $3`); a zero-argument
function referencing `$1`; a zero-argument function referencing a named `$a`;
and a positive case (`RETURN $1 + $2`) that must still be accepted.
- Verified: pre-fix the new tests fail (CREATE succeeded), post-fix all
pass. `cargo test -p datafusion --test user_defined_integration`, `cargo test
-p datafusion-sql`, `cargo fmt --check`, and clippy (`-D warnings`) on the
touched crates are all green. (Three `user_defined_integration` tests fail on
this machine only due to uninitialized `testing`/`parquet-testing` git
submodules — unrelated, pre-existing.)
--
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]
