Add support for procedure parameter default values #2041
Conversation
aharpervc
force-pushed
the
mssql-param-default-value
branch
2 times, most recently
from
f7f6284 to
6e18938
Compare
tests/sqlparser_common.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn create_procedure_with_parameter_default_value() { |
There was a problem hiding this comment.
Can we merge the test into the existing parse_create_procedure_with_parameter_modes? since they cover the same feature
There was a problem hiding this comment.
Hm, I don't understand. A parameter can be in/out and still have/not have a default, right? Are you suggesting like
pub enum ArgMode {
In,
Out,
InOut,
+ InWithDefault(Expr),
+ OutWithDefault(Expr),
+ InOutWithDefault(Expr),
}Perhaps I'm not understanding
There was a problem hiding this comment.
Ah no I meant to only inline this test case into that parse_create_procedure_with_parameter_modes function vs having one function per test case
There was a problem hiding this comment.
Oh yes, I understand now 👍
tests/sqlparser_common.rs
Outdated
|
|
||
| #[test] | ||
| fn create_procedure_with_parameter_default_value() { | ||
| let sql = r#"CREATE PROCEDURE test_proc (a INT = 42) AS BEGIN SELECT 1; END"#; |
There was a problem hiding this comment.
Can we add scenarios demonstrating
- multiple paramenters having defaults (i.e.
(a INT= 42, b INT=43)) - parameters that include a
mode
There was a problem hiding this comment.
Done 👍. I've also added an additional unit test for SQL Server, since the parameter identifier syntax is different there.
aharpervc
force-pushed
the
mssql-param-default-value
branch
from
6e18938 to
d05b63c
Compare
aharpervc
force-pushed
the
mssql-param-default-value
branch
from
d05b63c to
88da3b7
Compare
tests/sqlparser_common.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn create_procedure_with_parameter_default_value() { |
There was a problem hiding this comment.
Ah no I meant to only inline this test case into that parse_create_procedure_with_parameter_modes function vs having one function per test case
tests/sqlparser_common.rs
Outdated
| name: Ident { | ||
| value: "a".into(), | ||
| quote_style: None, | ||
| span: fake_span, | ||
| }, |
There was a problem hiding this comment.
| name: Ident { | |
| value: "a".into(), | |
| quote_style: None, | |
| span: fake_span, | |
| }, | |
| name: Ident::new("a"), |
I think this can be simplified? that would let us skip introducing the fake span as well
There was a problem hiding this comment.
Yes, certainly. I did it this way because that's how the similar modes test did it, but I will simplify
tests/sqlparser_mssql.rs
Outdated
| #[test] | ||
| fn parse_mssql_create_procedure_with_parameter_default_value() { | ||
| let sql = r#"CREATE PROCEDURE foo (IN @a INTEGER = 1, OUT @b TEXT = '2', INOUT @c DATETIME = NULL, @d BOOL = 0) AS BEGIN SELECT 1; END"#; | ||
| let _ = ms().verified_stmt(sql); | ||
| } |
There was a problem hiding this comment.
Similar comment here that we can inline this scenario into the parse_mssql_create_procedure function above
For example, this now parses appropriately:
Syntax documentation reference: https://learn.microsoft.com/en-us/sql/t-sql/statements/create-procedure-transact-sql?view=sql-server-ver17#e-use-a-procedure-with-wildcard-parameters. This is the syntax used by SQL Server, but I've implemented this as a "normal" Option property of the ProcedureParam struct.
I also added a corresponding common unit test