18450: feat: support named variables & defaults for CREATE FUNCTION
#3
Conversation
WalkthroughImplements support for defaulted and named arguments in user-defined scalar functions. Extends ScalarFunctionWrapper to hold defaults, updates placeholder handling to support named placeholders with parameter mapping, and adds validation for function argument consistency and default value ordering. Changes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧬 Code graph analysis (3)datafusion/sql/src/expr/value.rs (1)
datafusion/sql/src/statement.rs (3)
datafusion/core/tests/user_defined/user_defined_scalar_functions.rs (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| "Function argument {} not provided, argument missing!", | ||
| placeholder.id | ||
| )?, | ||
| } |
There was a problem hiding this comment.
Bug: Guarded Access to Parameter Defaults
Potential out-of-bounds array access in ScalarFunctionWrapper::replacement(). The code accesses defaults[placeholder_position] without checking if placeholder_position < defaults.len(). If a function body references a placeholder (e.g., $3) that doesn't correspond to a defined parameter, this will panic with an out-of-bounds error. A bounds check should be added before accessing the defaults array.
| .await | ||
| .expect_err("cannot mix named and positional style"); | ||
| let expected = "Error during planning: All function arguments must use either named or positional style."; | ||
| assert!(expected.starts_with(&err.strip_backtrace())); |
There was a problem hiding this comment.
Bug: Swapped Prefix Check for Error Messages
The arguments to starts_with() are reversed. The code checks expected.starts_with(&err.strip_backtrace()), which checks if the expected error message starts with the actual error. This should be err.strip_backtrace().starts_with(expected) to check if the actual error starts with the expected prefix, or use assert_eq! for exact comparison.
| .expect_err("non-default argument cannot follow default argument"); | ||
| let expected = | ||
| "Error during planning: Non-default arguments cannot follow default arguments."; | ||
| assert!(expected.starts_with(&err.strip_backtrace())); |
There was a problem hiding this comment.
Bug: Reversed prefixes in starts_with assertion
The arguments to starts_with() are reversed. The code checks expected.starts_with(&err.strip_backtrace()), which checks if the expected error message starts with the actual error. This should be err.strip_backtrace().starts_with(expected) to check if the actual error starts with the expected prefix, or use assert_eq! for exact comparison.
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
18450: To review by AI
Note
Adds named and default arguments to CREATE FUNCTION with validation and signature generation, supports named ($foo) placeholders, and updates tests/error messages.
$a,$b) and default argument expressions; validate style (all named vs all positional) and disallow non-defaults after defaults.arg_typesand enforce parameter-style consistency.TypeSignature::one_ofand store defaults; use defaults during expression simplification when args are omitted.$foo) alongside positional; for PREPARE, map known names to positional, otherwise error withUnknown placeholder.ScalarFunctionWrapper):defaultshandling and use it to replace missing args; update simplify replacement logic.CREATE FUNCTION, covering arity and validation errors.Unknown placeholdererrors and new behaviors.Written by Cursor Bugbot for commit 8ddaae0. This will update automatically on new commits. Configure here.