From 681dead1923a51aedc1da472bdffda6541d8adbb Mon Sep 17 00:00:00 2001 From: TCeason Date: Wed, 5 Nov 2025 15:12:28 +0800 Subject: [PATCH 1/2] fix(query): handle complex types in procedure argument parsing Fixed procedure argument type parsing to correctly handle types with nested parentheses like Decimal(4, 2). --- .../sql/src/planner/binder/ddl/procedure.rs | 37 ++++++++++++++++++- .../base/15_procedure/15_0002_procedure.test | 14 ++++++- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/query/sql/src/planner/binder/ddl/procedure.rs b/src/query/sql/src/planner/binder/ddl/procedure.rs index acd65cad271af..78c56a7cfcf76 100644 --- a/src/query/sql/src/planner/binder/ddl/procedure.rs +++ b/src/query/sql/src/planner/binder/ddl/procedure.rs @@ -266,9 +266,10 @@ fn generate_procedure_name_ident( return Ok(ProcedureNameIdent::new(tenant, name.clone().into())); } + let args = split_procedure_arg_types(&name.args_type)?; let mut args_type = vec![]; - for arg in name.args_type.split(',') { - args_type.push(DataType::from(&resolve_type_name_by_str(arg, true)?)); + for arg in args { + args_type.push(DataType::from(&resolve_type_name_by_str(&arg, true)?)); } let new_name = databend_common_ast::ast::ProcedureIdentity { name: name.name.to_string(), @@ -283,3 +284,35 @@ fn generate_procedure_name_ident( ProcedureIdentity::from(new_name), )) } + +fn split_procedure_arg_types(raw: &str) -> Result> { + let mut parts = Vec::new(); + let mut depth = 0; + let mut start = 0; + + for (i, c) in raw.char_indices() { + match c { + '(' => depth += 1, + ')' => depth -= 1, + ',' if depth == 0 => { + let arg = raw[start..i].trim(); + if !arg.is_empty() { + parts.push(arg.to_string()); + } + start = i + 1; + } + _ => {} + } + } + + if depth != 0 { + return Err(ErrorCode::SyntaxException("unmatched parentheses")); + } + + let last = raw[start..].trim(); + if !last.is_empty() { + parts.push(last.to_string()); + } + + Ok(parts) +} diff --git a/tests/sqllogictests/suites/base/15_procedure/15_0002_procedure.test b/tests/sqllogictests/suites/base/15_procedure/15_0002_procedure.test index 92424863d5374..8b0cac90c0360 100644 --- a/tests/sqllogictests/suites/base/15_procedure/15_0002_procedure.test +++ b/tests/sqllogictests/suites/base/15_procedure/15_0002_procedure.test @@ -132,6 +132,19 @@ drop procedure p1(UInt8, UInt8); statement ok drop procedure p1(int); +statement ok +CREATE OR REPLACE PROCEDURE p_decimal_arg(x Decimal(4, 2)) RETURNS Decimal(4, 2) LANGUAGE SQL COMMENT='decimal arg' AS $$ +BEGIN + RETURN x; +END; +$$; + +statement ok +desc procedure p_decimal_arg(Decimal(4, 2)); + +statement ok +drop procedure p_decimal_arg(Decimal(4, 2)); + statement ok drop procedure if exists not_exists_p(); @@ -203,4 +216,3 @@ call procedure p3('x'); statement ok drop procedure p3(string); - From b465ed16308b67ad4b4cc93d830d2c68dae02568 Mon Sep 17 00:00:00 2001 From: TCeason Date: Thu, 6 Nov 2025 11:27:48 +0800 Subject: [PATCH 2/2] refactor(ast): use Vec in ProcedureIdentity instead of String This change improves type safety by preserving TypeName information from the parser to the binder, avoiding unnecessary string round-trip conversions when handling complex types like Decimal(4, 2). --- .../app/src/principal/procedure_identity.rs | 8 ++- src/query/ast/src/ast/statements/procedure.rs | 32 +++++++++- src/query/ast/src/ast/statements/user.rs | 8 +-- src/query/ast/src/parser/statement.rs | 61 +++---------------- .../sql/src/planner/binder/ddl/procedure.rs | 61 +++++-------------- 5 files changed, 62 insertions(+), 108 deletions(-) diff --git a/src/meta/app/src/principal/procedure_identity.rs b/src/meta/app/src/principal/procedure_identity.rs index 2ce09629e670b..2093085de4bda 100644 --- a/src/meta/app/src/principal/procedure_identity.rs +++ b/src/meta/app/src/principal/procedure_identity.rs @@ -64,6 +64,12 @@ impl KeyCodec for ProcedureIdentity { impl From for ProcedureIdentity { fn from(procedure: databend_common_ast::ast::ProcedureIdentity) -> Self { - ProcedureIdentity::new(procedure.name, procedure.args_type) + let args_type_str = procedure + .args_type + .iter() + .map(|t| t.to_string()) + .collect::>() + .join(","); + ProcedureIdentity::new(procedure.name, args_type_str) } } diff --git a/src/query/ast/src/ast/statements/procedure.rs b/src/query/ast/src/ast/statements/procedure.rs index 1931edc29cc02..0467718ae1309 100644 --- a/src/query/ast/src/ast/statements/procedure.rs +++ b/src/query/ast/src/ast/statements/procedure.rs @@ -65,15 +65,41 @@ impl Display for ProcedureLanguage { } } -#[derive(Debug, Clone, PartialEq, Eq, Drive, DriveMut)] +#[derive(Clone, PartialEq, Drive, DriveMut)] pub struct ProcedureIdentity { pub name: String, - pub args_type: String, + pub args_type: Vec, +} + +impl std::fmt::Debug for ProcedureIdentity { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ProcedureIdentity") + .field("name", &self.name) + .field( + "args_type", + &self + .args_type + .iter() + .map(|t| t.to_string()) + .collect::>() + .join(","), + ) + .finish() + } } impl Display for ProcedureIdentity { fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { - write!(f, "{}({})", &self.name, &self.args_type,) + write!( + f, + "{}({})", + &self.name, + self.args_type + .iter() + .map(|t| t.to_string()) + .collect::>() + .join(",") + ) } } diff --git a/src/query/ast/src/ast/statements/user.rs b/src/query/ast/src/ast/statements/user.rs index ad802cbb103ce..eb7467c0982f5 100644 --- a/src/query/ast/src/ast/statements/user.rs +++ b/src/query/ast/src/ast/statements/user.rs @@ -104,7 +104,7 @@ impl Display for AlterUserStmt { } } -#[derive(Debug, Clone, PartialEq, Eq, Drive, DriveMut)] +#[derive(Debug, Clone, PartialEq, Drive, DriveMut)] pub struct GrantStmt { pub source: AccountMgrSource, pub principal: PrincipalIdentity, @@ -120,7 +120,7 @@ impl Display for GrantStmt { } } -#[derive(Debug, Clone, PartialEq, Eq, Drive, DriveMut)] +#[derive(Debug, Clone, PartialEq, Drive, DriveMut)] pub struct RevokeStmt { pub source: AccountMgrSource, pub principal: PrincipalIdentity, @@ -205,7 +205,7 @@ impl Display for ShowObjectPrivilegesStmt { } } -#[derive(Debug, Clone, PartialEq, Eq, Drive, DriveMut)] +#[derive(Debug, Clone, PartialEq, Drive, DriveMut)] pub enum AccountMgrSource { Role { role: String, @@ -239,7 +239,7 @@ impl Display for AccountMgrSource { } } -#[derive(Debug, Clone, PartialEq, Eq, Drive, DriveMut)] +#[derive(Debug, Clone, PartialEq, Drive, DriveMut)] pub enum AccountMgrLevel { Global, Database(Option), diff --git a/src/query/ast/src/parser/statement.rs b/src/query/ast/src/parser/statement.rs index 53896aebec130..dec64a8e920a7 100644 --- a/src/query/ast/src/parser/statement.rs +++ b/src/query/ast/src/parser/statement.rs @@ -2489,12 +2489,9 @@ pub fn statement_body(i: Input) -> IResult { let name = ProcedureIdentity { name: name.to_string(), args_type: if let Some(args) = &args { - args.iter() - .map(|arg| arg.data_type.to_string()) - .collect::>() - .join(",") + args.iter().map(|arg| arg.data_type.clone()).collect() } else { - "".to_string() + vec![] }, }; let stmt = CreateProcedureStmt { @@ -2558,14 +2555,7 @@ pub fn statement_body(i: Input) -> IResult { if_exists: opt_if_exists.is_some(), name: ProcedureIdentity { name: name.to_string(), - args_type: if args.is_empty() { - "".to_string() - } else { - args.iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(",") - }, + args_type: args, }, }) }, @@ -2579,14 +2569,7 @@ pub fn statement_body(i: Input) -> IResult { Statement::DescProcedure(DescProcedureStmt { name: ProcedureIdentity { name: name.to_string(), - args_type: if args.is_empty() { - "".to_string() - } else { - args.iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(",") - }, + args_type: args, }, }) }, @@ -3636,14 +3619,7 @@ pub fn grant_source(i: Input) -> IResult { privileges: vec![UserPrivilegeType::AccessProcedure], level: AccountMgrLevel::Procedure(ProcedureIdentity { name: name.to_string(), - args_type: if args.is_empty() { - "".to_string() - } else { - args.iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(",") - }, + args_type: args, }), }, ); @@ -3656,14 +3632,7 @@ pub fn grant_source(i: Input) -> IResult { privileges: vec![UserPrivilegeType::AccessProcedure], level: AccountMgrLevel::Procedure(ProcedureIdentity { name: name.to_string(), - args_type: if args.is_empty() { - "".to_string() - } else { - args.iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(",") - }, + args_type: args, }), }, ); @@ -3833,14 +3802,7 @@ pub fn on_object_name(i: Input) -> IResult { |(_, name, args)| { GrantObjectName::Procedure(ProcedureIdentity { name: name.to_string(), - args_type: if args.is_empty() { - "".to_string() - } else { - args.iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(",") - }, + args_type: args, }) }, ); @@ -3979,14 +3941,7 @@ pub fn grant_ownership_level(i: Input) -> IResult { |(_, name, args)| { let name = ProcedureIdentity { name: name.to_string(), - args_type: if args.is_empty() { - "".to_string() - } else { - args.iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(",") - }, + args_type: args, }; AccountMgrLevel::Procedure(name) }, diff --git a/src/query/sql/src/planner/binder/ddl/procedure.rs b/src/query/sql/src/planner/binder/ddl/procedure.rs index 78c56a7cfcf76..78da7d0955321 100644 --- a/src/query/sql/src/planner/binder/ddl/procedure.rs +++ b/src/query/sql/src/planner/binder/ddl/procedure.rs @@ -49,7 +49,6 @@ use crate::plans::Plan; use crate::plans::RewriteKind; use crate::plans::SubqueryType; use crate::resolve_type_name; -use crate::resolve_type_name_by_str; use crate::BindContext; use crate::Binder; use crate::ScalarExpr; @@ -266,53 +265,21 @@ fn generate_procedure_name_ident( return Ok(ProcedureNameIdent::new(tenant, name.clone().into())); } - let args = split_procedure_arg_types(&name.args_type)?; - let mut args_type = vec![]; - for arg in args { - args_type.push(DataType::from(&resolve_type_name_by_str(&arg, true)?)); - } - let new_name = databend_common_ast::ast::ProcedureIdentity { - name: name.name.to_string(), - args_type: args_type - .iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(","), - }; + let args_data_type: Vec = name + .args_type + .iter() + .map(|type_name| resolve_type_name(type_name, true).map(|t| DataType::from(&t))) + .collect::, _>>()?; + + // Convert normalized DataType back to string for storage + let args_type_str = args_data_type + .iter() + .map(|dt| dt.to_string()) + .collect::>() + .join(","); + Ok(ProcedureNameIdent::new( tenant, - ProcedureIdentity::from(new_name), + ProcedureIdentity::new(name.name.clone(), args_type_str), )) } - -fn split_procedure_arg_types(raw: &str) -> Result> { - let mut parts = Vec::new(); - let mut depth = 0; - let mut start = 0; - - for (i, c) in raw.char_indices() { - match c { - '(' => depth += 1, - ')' => depth -= 1, - ',' if depth == 0 => { - let arg = raw[start..i].trim(); - if !arg.is_empty() { - parts.push(arg.to_string()); - } - start = i + 1; - } - _ => {} - } - } - - if depth != 0 { - return Err(ErrorCode::SyntaxException("unmatched parentheses")); - } - - let last = raw[start..].trim(); - if !last.is_empty() { - parts.push(last.to_string()); - } - - Ok(parts) -}