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

CREATE FUNCTION accepted placeholders that did not match a declared argument, such as $3 in a function with two arguments. The error only appeared when the function was called.

This PR rejects these invalid definitions when the function is created.

What changes are included?

• Adds placeholder validation to CreateFunction::try_new, keeping the semantic check in the logical-expression layer rather than the SQL planner.
• Validates placeholders in function bodies, argument defaults, and nested subqueries.
• Keeps valid placeholders and PREPARE parameter handling unchanged.
• Adds end-to-end SQL logic tests and focused unit coverage for nested subqueries.

User-facing change

Invalid placeholders now produce a planning error during CREATE FUNCTION instead of failing later when the function is called.

[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 97.02970% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.42%. Comparing base (c149764) to head (844f804).
⚠️ Report is 145 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25039      +/-   ##
==========================================
+ Coverage   81.91%   82.42%   +0.50%     
==========================================
  Files        1134     1138       +4     
  Lines      425703   435504    +9801     
  Branches   425703   435504    +9801     
==========================================
+ Hits       348725   358951   +10226     
+ Misses      56300    54849    -1451     
- Partials    20678    21704    +1026     

☔ 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.

[alamb]

I will review