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

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs

@@ -1125,23 +1125,81 @@ 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
+    // Definitions with 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("select better_add(2.0, 2.0)")
-        .await?
-        .collect()
+        .sql(bad_expression_sql)
         .await
-        .expect_err("unknown placeholder");
-    let expected = "Optimizer rule 'simplify_expressions' failed\ncaused by\nExecution error: Invalid placeholder, out of range: $3";
+        .expect_err("invalid placeholder");
+    let expected = "Error during planning: Invalid placeholder, out of range: $3";
+    assert!(expected.starts_with(&err.strip_backtrace()));
+
+    Ok(())
+}
+
+#[tokio::test]
+async fn create_scalar_function_from_sql_statement_invalid_placeholders() -> Result<()> {
+    let function_factory = Arc::new(CustomFunctionFactory::default());
+    let ctx = SessionContext::new().with_function_factory(function_factory.clone());
+
+    // Out of range positional placeholder in the body of a function declared
+    // with positional arguments
+    let sql = r#"
+    CREATE FUNCTION bad_placeholder_pos(DOUBLE, DOUBLE)
+        RETURNS DOUBLE
+        RETURN $1 + $3
+    "#;
+    let err = ctx.sql(sql).await.expect_err("out of range placeholder");
+    let expected = "Error during planning: Invalid placeholder, out of range: $3";
+    assert!(expected.starts_with(&err.strip_backtrace()));
+
+    // Out of range positional placeholder in the body of a function declared
+    // with named arguments
+    let sql = r#"
+    CREATE FUNCTION bad_placeholder_named(a DOUBLE, b DOUBLE)
+        RETURNS DOUBLE
+        RETURN $a + $3
+    "#;
+    let err = ctx.sql(sql).await.expect_err("out of range placeholder");
+    let expected = "Error during planning: Invalid placeholder, out of range: $3";
+    assert!(expected.starts_with(&err.strip_backtrace()));
+
+    // Placeholder in the body of a function declared with no arguments
+    let sql = r#"
+    CREATE FUNCTION bad_placeholder_zero_args()
+        RETURNS DOUBLE
+        RETURN $1
+    "#;
+    let err = ctx.sql(sql).await.expect_err("out of range placeholder");
+    let expected = "Error during planning: Invalid placeholder, out of range: $1";
+    assert!(expected.starts_with(&err.strip_backtrace()));
+
+    // Named placeholder in the body of a function declared with no arguments
+    let sql = r#"
+    CREATE FUNCTION bad_placeholder_unknown_name()
+        RETURNS DOUBLE
+        RETURN $a
+    "#;
+    let err = ctx.sql(sql).await.expect_err("unknown placeholder");
+    let expected = "Error during planning: Unknown placeholder: $a";
     assert!(expected.starts_with(&err.strip_backtrace()));
 
+    // Valid bodies must still be accepted
+    let sql = r#"
+    CREATE FUNCTION good_placeholder(DOUBLE, DOUBLE)
+        RETURNS DOUBLE
+        RETURN $1 + $2
+    "#;
+    assert!(ctx.sql(sql).await.is_ok());
+
+    // A function without a body has no placeholders to validate
+    let sql = "CREATE FUNCTION no_body() RETURNS DOUBLE";
+    ctx.state().create_logical_plan(sql).await?;
+
     Ok(())
 }
 

datafusion/sql/src/expr/value.rs

@@ -155,7 +155,10 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
             }
         };
         // Check if the placeholder is in the parameter list
-        // FIXME: In the CREATE FUNCTION branch, param_type = None should raise an error
+        // An out of range placeholder for CREATE FUNCTION is rejected at the
+        // statement level (see the `Statement::CreateFunction` arm of
+        // `statement.rs`). Here, a missing parameter type is left permissive so
+        // that PREPARE can defer type inference to bind time.
         let param_type = param_data_types.get(idx).and_then(|v| v.clone());
         // Data type of the parameter
         debug!("type of param {param} param_data_types[idx]: {param_type:?}");

datafusion/sql/src/statement.rs

@@ -1462,6 +1462,45 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                     None => None,
                 };
 
+                // Reject placeholders in the RETURN body that do not reference
+                // a declared argument. Without this check, an invalid
+                // definition such as `RETURN $1 + $3` for a function declared
+                // with two arguments is only rejected when the function is
+                // invoked (by FunctionFactories that substitute placeholders
+                // at call time), rather than at CREATE FUNCTION time.
+                if let Some(body) = &function_body {
+                    let arg_count = args.as_ref().map_or(0, |declared| declared.len());
+                    body.apply(|expr| {
+                        if let Expr::Placeholder(placeholder) = expr {
+                            match placeholder
+                                .id
+                                .strip_prefix('$')
+                                .and_then(|id| id.parse::<usize>().ok())
+                            {
+                                // Positional placeholder within the declared
+                                // argument list (e.g. `$2` with two arguments)
+                                Some(idx) if (1..=arg_count).contains(&idx) => {}
+                                // Positional placeholder that is out of range
+                                Some(_) => {
+                                    return plan_err!(
+                                        "Invalid placeholder, out of range: {}",
+                                        placeholder.id
+                                    );
+                                }
+                                // A named placeholder can only survive parsing
+                                // when no arguments were declared
+                                None => {
+                                    return plan_err!(
+                                        "Unknown placeholder: {}",
+                                        placeholder.id
+                                    );
+                                }
+                            }
+                        }
+                        Ok(TreeNodeRecursion::Continue)
+                    })?;
+                }
+
                 let params = CreateFunctionBody {
                     language,
                     behavior: behavior.map(|b| match b {

datafusion/sqllogictest/test_files/create_function.slt

@@ -23,8 +23,17 @@
 
 # Create function will fail unless a user supplied function factory is supplied
 statement error DataFusion error: Invalid or Unsupported Configuration: Function factory has not been configured
+CREATE FUNCTION foo (DOUBLE) RETURNS DOUBLE RETURN $1;
+
+# Invalid placeholders in the body are rejected during planning, before a
+# function factory is consulted (see issue #25038)
+statement error DataFusion error: Error during planning: Invalid placeholder, out of range: \$2
 CREATE FUNCTION foo (DOUBLE) RETURNS DOUBLE RETURN $1 + $2;
 
+# A zero-argument function cannot reference arguments in its body either
+statement error DataFusion error: Error during planning: Invalid placeholder, out of range: \$1
+CREATE FUNCTION foo_zero () RETURNS DOUBLE RETURN $1;
+
 # multi-part identifiers are not supported
 statement error DataFusion error: This feature is not implemented: Qualified functions are not supported
 CREATE FUNCTION foo.bar (DOUBLE) RETURNS DOUBLE RETURN $1 + $2;