fix: reject invalid placeholders in CREATE FUNCTION bodies at definition time

[quwin]

Which issue does this PR close?

• Closes CREATE FUNCTION accepts placeholders that don't match a declared argument; the error is deferred to call time #25038.

Rationale for this change

A CREATE FUNCTION with a SQL body was accepted even when its RETURN expression referenced a placeholder that does not match a declared argument:

CREATE FUNCTION better_add(DOUBLE, DOUBLE)
RETURNS DOUBLE
RETURN $1 + $3 -- only two arguments are declared

The invalid definition was registered successfully and the error only surfaced when the function was invoked (Invalid placeholder, out of range: $3), deferring a definition error to every call. This PR rejects such definitions at CREATE FUNCTION time.

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) via a validate_function_body_placeholders helper that rejects:

• positional $N outside 1..=declared_arg_count with Invalid placeholder, out of range: $N
• named placeholders (only possible with zero declared arguments) with Unknown placeholder: $N

The validation covers:

• the RETURN body, including inside subqueries: Expr::apply treats Expr::ScalarSubquery, Expr::Exists, Expr::InSubquery, and Expr::SetComparison as leaves, so the helper additionally walks each subquery's plan with apply_with_subqueries + apply_expressions, mirroring LogicalPlan::get_parameter_fields
• argument DEFAULT expressions, which are spliced into the body at call time

Doing this in the planner (rather than in value.rs or in the factories) applies to every FunctionFactory implementation and cannot change PREPARE semantics, where an empty/unknown parameter list must stay permissive for deferred type inference. Functions with defaulted arguments remain callable with fewer arguments than declared, since the check uses the declared argument count. Replaced the now-superseded FIXME in datafusion/sql/src/expr/value.rs.

What is the testing strategy for this PR?

create_scalar_function_from_sql_statement_invalid_placeholders() covers out-of-range positional placeholders with positional and named declared args, zero-argument functions, named placeholders in default expressions, and out-of-range placeholders inside scalar subqueries, EXISTS, IN, subqueries nested two levels deep, and a subquery's ORDER BY/LIMIT — plus positive cases proving valid placeholders in bodies, defaults, and subqueries are still accepted. create_function.slt asserts the new definition-time errors explicitly.

cargo test -p datafusion --test user_defined_integration, cargo test -p datafusion-sql, cargo test -p datafusion-sqllogictest --test sqllogictests create_function, cargo fmt --check, and clippy (-D warnings) on the touched crates are all green.

Are there any user-facing changes?

Yes: CREATE FUNCTION definitions with a placeholder that does not reference a declared argument — in the body or an argument default, including inside subqueries — now fail at definition time with a planning error instead of failing at every invocation.

[quwin]

Hi, this is my first PR to DataFusion. Could a committer please run the CI checks? Thanks!
@jayzhan211

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.10526% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.91%. Comparing base (c149764) to head (fd77ff8).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/sql/src/statement.rs	92.10%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25039      +/-   ##
==========================================
- Coverage   81.91%   81.91%   -0.01%     
==========================================
  Files        1134     1134              
  Lines      425703   425746      +43     
  Branches   425703   425746      +43     
==========================================
+ Hits       348725   348750      +25     
- Misses      56300    56314      +14     
- Partials    20678    20682       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[quwin]

I've addressed the Codecov report by adding coverage for the body-less CREATE FUNCTION path. Apologies if another ping is unnecessary, but could a committer please approve the new CI run? Thanks!
@jayzhan211

[jayzhan211]

Thanks @quwin, here is a suggestion:

Expr::apply doesn't descend into subqueries — apply_children treats
Expr::ScalarSubquery / Expr::InSubquery / Expr::Exists as leaves
(datafusion/expr/src/tree_node.rs:83).
So a RETURN body with a subquery still slips an invalid placeholder past the
new check. On this branch:

CREATE FUNCTION p3(DOUBLE) RETURNS DOUBLE RETURN $1 + (SELECT $9);  -- accepted
SELECT p3(1.0);
-- Execution error: Placeholder '$9' was not provided a value for execution.

That's the same failure #25038 describes: bad definition registered, error
deferred to every call.

The codebase's own placeholder walk (LogicalPlan::get_parameter_fields,
datafusion/expr/src/logical_plan/plan.rs:1842) pairs apply_with_subqueries
with apply_expressions for exactly this reason. Suggest extracting the check
into a helper and doing the same:

+fn validate_function_body_placeholders(expr: &Expr, arg_count: usize) -> Result<()> {
+    expr.apply(|expr| {
+        match expr {
+            Expr::Placeholder(placeholder) => {
+                match placeholder
+                    .id
+                    .strip_prefix('$')
+                    .and_then(|id| id.parse::<usize>().ok())
+                {
+                    Some(idx) if (1..=arg_count).contains(&idx) => {}
+                    Some(_) => {
+                        return plan_err!(
+                            "Invalid placeholder, out of range: {}",
+                            placeholder.id
+                        );
+                    }
+                    None => {
+                        return plan_err!("Unknown placeholder: {}", placeholder.id);
+                    }
+                }
+            }
+            // `Expr::apply` stops at subquery boundaries; walk the subquery's
+            // plan so placeholders inside it are validated too.
+            Expr::ScalarSubquery(subquery)
+            | Expr::Exists(Exists { subquery, .. })
+            | Expr::InSubquery(InSubquery { subquery, .. }) => {
+                subquery.subquery.apply_with_subqueries(|plan| {
+                    plan.apply_expressions(|e| {
+                        validate_function_body_placeholders(e, arg_count)?;
+                        Ok(TreeNodeRecursion::Continue)
+                    })
+                })?;
+            }
+            _ => {}
+        }
+        Ok(TreeNodeRecursion::Continue)
+    })?;
+    Ok(())
+}

and in the Statement::CreateFunction arm:

 if let Some(body) = &function_body {
     let arg_count = args.as_ref().map_or(0, |declared| declared.len());
-    body.apply(|expr| {
-        ...
-    })?;
+    validate_function_body_placeholders(body, arg_count)?;
 }

Worth adding the subquery case to
create_scalar_function_from_sql_statement_invalid_placeholders so it stays
covered:

let sql = r#"
CREATE FUNCTION bad_placeholder_subquery(DOUBLE)
    RETURNS DOUBLE
    RETURN $1 + (SELECT $9)
"#;
let err = ctx.sql(sql).await.expect_err("out of range placeholder");
assert_eq!(
    err.strip_backtrace(),
    "Error during planning: Invalid placeholder, out of range: $9"
);

[quwin]

Thanks for the review @jayzhan211,

I've extracted validate_function_body_placeholders and walked subqueries with apply_with_subqueries + apply_expressions as you suggested, covering ScalarSubquery, Exists, InSubquery, and SetComparison.

As a follow-up, I found a second gap, where argument DEFAULT expressions are planned permissively and spliced into the body at call time, so DEFAULT $9 also slipped through. They're now validated with the same rule.

For tests, they now cover subquery forms, nesting two levels deep, subquery ORDER BY/ LIMIT, defaults, and positive cases; .slt asserts the definition-time errors.

Could you re-run CI and take a look at the updated head? Thanks!

[jayzhan211]

Thanks @quwin !

[alamb]

Thanks for this PR @quwin and @jayzhan211

[quwin]

Thanks @alamb ! The requested changes are now pushed: semantic validation is in the logical-expression layer, and the end-to-end checks are covered by sqllogictest. Please re-review the updated head when convenient.