From 69d9328263dc31d507bf9e5a30bfc37442ae2188 Mon Sep 17 00:00:00 2001 From: LucaCappelletti94 Date: Thu, 12 Nov 2026 20:44:26 +0100 Subject: [PATCH] PostgreSQL: Represent `ALTER AGGREGATE` signatures with one type --- src/ast/ddl.rs | 106 ++++++++++++++++++---------------- src/ast/mod.rs | 60 +++++++++---------- src/parser/mod.rs | 63 +++++++++----------- tests/sqlparser_postgres.rs | 111 +++++++++++++++++++++++++++++++++--- 4 files changed, 217 insertions(+), 123 deletions(-) diff --git a/src/ast/ddl.rs b/src/ast/ddl.rs index a6a8dc7f9e..526de62e6f 100644 --- a/src/ast/ddl.rs +++ b/src/ast/ddl.rs @@ -6087,43 +6087,79 @@ impl Spanned for AlterOperatorClass { } } +/// An aggregate's name and argument signature. +#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] +#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] +#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] +pub struct AggregateSignature { + /// Aggregate name. + pub name: ObjectName, + /// Aggregate arguments. + pub args: AggregateArgs, +} + +/// Arguments identifying an aggregate. +#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] +#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] +#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] +pub enum AggregateArgs { + /// The `*` signature. + Star, + /// Direct arguments with an optional ordered-set argument list. + List { + /// Direct arguments. + args: Vec, + /// Arguments following `ORDER BY`. + order_by: Option>, + }, +} + +impl fmt::Display for AggregateSignature { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}(", self.name)?; + match &self.args { + AggregateArgs::Star => write!(f, "*")?, + AggregateArgs::List { args, order_by } => { + write!(f, "{}", display_comma_separated(args))?; + if let Some(order_by) = order_by { + if !args.is_empty() { + write!(f, " ")?; + } + write!(f, "ORDER BY {}", display_comma_separated(order_by))?; + } + } + } + write!(f, ")") + } +} + /// `ALTER FUNCTION` / `ALTER AGGREGATE` statement. #[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] pub struct AlterFunction { - /// Object type being altered. - pub kind: AlterFunctionKind, /// Function or aggregate signature. - pub function: FunctionDesc, - /// `ORDER BY` argument list for aggregate signatures. - /// - /// This is only used for `ALTER AGGREGATE`. - pub aggregate_order_by: Option>, - /// Whether the aggregate signature uses `*`. - /// - /// This is only used for `ALTER AGGREGATE`. - pub aggregate_star: bool, + pub signature: AlterFunctionSignature, /// Operation applied to the object. pub operation: AlterFunctionOperation, } -/// Function-like object type used by [`AlterFunction`]. +/// The function or aggregate identified by [`AlterFunction`]. #[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] -pub enum AlterFunctionKind { - /// `FUNCTION` - Function, - /// `AGGREGATE` - Aggregate, +pub enum AlterFunctionSignature { + /// A function signature. + Function(FunctionDesc), + /// An aggregate signature. + Aggregate(AggregateSignature), } -impl fmt::Display for AlterFunctionKind { +impl fmt::Display for AlterFunctionSignature { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::Function => write!(f, "FUNCTION"), - Self::Aggregate => write!(f, "AGGREGATE"), + Self::Function(function) => write!(f, "FUNCTION {function}"), + Self::Aggregate(signature) => write!(f, "AGGREGATE {signature}"), } } } @@ -6196,35 +6232,7 @@ pub enum AlterFunctionAction { impl fmt::Display for AlterFunction { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "ALTER {} ", self.kind)?; - match self.kind { - AlterFunctionKind::Function => { - write!(f, "{} ", self.function)?; - } - AlterFunctionKind::Aggregate => { - write!(f, "{}(", self.function.name)?; - if self.aggregate_star { - write!(f, "*")?; - } else { - if let Some(args) = &self.function.args { - write!(f, "{}", display_comma_separated(args))?; - } - if let Some(order_by_args) = &self.aggregate_order_by { - if self - .function - .args - .as_ref() - .is_some_and(|args| !args.is_empty()) - { - write!(f, " ")?; - } - write!(f, "ORDER BY {}", display_comma_separated(order_by_args))?; - } - } - write!(f, ") ")?; - } - } - write!(f, "{}", self.operation) + write!(f, "ALTER {} {}", self.signature, self.operation) } } diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 9e5e865c22..7650eb8ae1 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -64,36 +64,36 @@ pub use self::dcl::{ SetConfigValue, Use, }; pub use self::ddl::{ - AggregateModifyKind, Alignment, AlterCollation, AlterCollationOperation, AlterColumnOperation, - AlterConnectorOwner, AlterExtension, AlterExtensionMember, AlterExtensionOperation, - AlterFunction, AlterFunctionAction, AlterFunctionKind, AlterFunctionOperation, - AlterGenericOption, AlterIndexOperation, AlterOperator, AlterOperatorClass, - AlterOperatorClassOperation, AlterOperatorFamily, AlterOperatorFamilyOperation, - AlterOperatorOperation, AlterPolicy, AlterPolicyOperation, AlterSchema, AlterSchemaOperation, - AlterTable, AlterTableAlgorithm, AlterTableLock, AlterTableOperation, AlterTableType, - AlterTextSearch, AlterTextSearchOperation, AlterTextSearchOption, AlterTrigger, - AlterTriggerOperation, AlterType, AlterTypeAddValue, AlterTypeAddValuePosition, - AlterTypeOperation, AlterTypeRename, AlterTypeRenameValue, AlterView, AlterViewOperation, - ClusteredBy, ColumnDef, ColumnOption, ColumnOptionDef, ColumnOptions, ColumnPolicy, - ColumnPolicyProperty, ConstraintCharacteristics, CreateAggregate, CreateAggregateArgs, - CreateAggregateOption, CreateCollation, CreateCollationDefinition, CreateConnector, - CreateDomain, CreateExtension, CreateForeignDataWrapper, CreateForeignTable, CreateFunction, - CreateIndex, CreateOperator, CreateOperatorClass, CreateOperatorFamily, CreatePolicy, - CreatePolicyCommand, CreatePolicyType, CreateRule, CreateRuleAction, CreateRuleDoKind, - CreateRuleEvent, CreateTable, CreateTextSearch, CreateTrigger, CreateUserMapping, CreateView, - Deduplicate, DeferrableInitial, DetachPartitionMode, DistStyle, DropBehavior, DropExtension, - DropFunction, DropOperator, DropOperatorClass, DropOperatorFamily, DropOperatorSignature, - DropPolicy, DropRule, DropTrigger, ForValues, ForeignDataWrapperRoutineClause, - FunctionReturnType, GeneratedAs, GeneratedExpressionMode, IdentityParameters, IdentityProperty, - IdentityPropertyFormatKind, IdentityPropertyKind, IdentityPropertyOrder, IndexColumn, - IndexOption, IndexType, KeyOrIndexDisplay, Msck, NullsDistinctOption, OperatorArgTypes, - OperatorClassItem, OperatorFamilyDropItem, OperatorFamilyItem, OperatorOption, OperatorPurpose, - Owner, Partition, PartitionBoundValue, ProcedureParam, ReferentialAction, RenameTableNameKind, - ReplicaIdentity, SqliteTableOption, SqliteTableOptions, TagsColumnOption, TextSearchObjectType, - TriggerObjectKind, Truncate, UserDefinedTypeCompositeAttributeDef, - UserDefinedTypeInternalLength, UserDefinedTypeRangeOption, UserDefinedTypeRepresentation, - UserDefinedTypeSqlDefinitionOption, UserDefinedTypeStorage, UserMappingUser, ViewColumnDef, - WithData, + AggregateArgs, AggregateModifyKind, AggregateSignature, Alignment, AlterCollation, + AlterCollationOperation, AlterColumnOperation, AlterConnectorOwner, AlterExtension, + AlterExtensionMember, AlterExtensionOperation, AlterFunction, AlterFunctionAction, + AlterFunctionOperation, AlterFunctionSignature, AlterGenericOption, AlterIndexOperation, + AlterOperator, AlterOperatorClass, AlterOperatorClassOperation, AlterOperatorFamily, + AlterOperatorFamilyOperation, AlterOperatorOperation, AlterPolicy, AlterPolicyOperation, + AlterSchema, AlterSchemaOperation, AlterTable, AlterTableAlgorithm, AlterTableLock, + AlterTableOperation, AlterTableType, AlterTextSearch, AlterTextSearchOperation, + AlterTextSearchOption, AlterTrigger, AlterTriggerOperation, AlterType, AlterTypeAddValue, + AlterTypeAddValuePosition, AlterTypeOperation, AlterTypeRename, AlterTypeRenameValue, + AlterView, AlterViewOperation, ClusteredBy, ColumnDef, ColumnOption, ColumnOptionDef, + ColumnOptions, ColumnPolicy, ColumnPolicyProperty, ConstraintCharacteristics, CreateAggregate, + CreateAggregateArgs, CreateAggregateOption, CreateCollation, CreateCollationDefinition, + CreateConnector, CreateDomain, CreateExtension, CreateForeignDataWrapper, CreateForeignTable, + CreateFunction, CreateIndex, CreateOperator, CreateOperatorClass, CreateOperatorFamily, + CreatePolicy, CreatePolicyCommand, CreatePolicyType, CreateRule, CreateRuleAction, + CreateRuleDoKind, CreateRuleEvent, CreateTable, CreateTextSearch, CreateTrigger, + CreateUserMapping, CreateView, Deduplicate, DeferrableInitial, DetachPartitionMode, DistStyle, + DropBehavior, DropExtension, DropFunction, DropOperator, DropOperatorClass, DropOperatorFamily, + DropOperatorSignature, DropPolicy, DropRule, DropTrigger, ForValues, + ForeignDataWrapperRoutineClause, FunctionReturnType, GeneratedAs, GeneratedExpressionMode, + IdentityParameters, IdentityProperty, IdentityPropertyFormatKind, IdentityPropertyKind, + IdentityPropertyOrder, IndexColumn, IndexOption, IndexType, KeyOrIndexDisplay, Msck, + NullsDistinctOption, OperatorArgTypes, OperatorClassItem, OperatorFamilyDropItem, + OperatorFamilyItem, OperatorOption, OperatorPurpose, Owner, Partition, PartitionBoundValue, + ProcedureParam, ReferentialAction, RenameTableNameKind, ReplicaIdentity, SqliteTableOption, + SqliteTableOptions, TagsColumnOption, TextSearchObjectType, TriggerObjectKind, Truncate, + UserDefinedTypeCompositeAttributeDef, UserDefinedTypeInternalLength, + UserDefinedTypeRangeOption, UserDefinedTypeRepresentation, UserDefinedTypeSqlDefinitionOption, + UserDefinedTypeStorage, UserMappingUser, ViewColumnDef, WithData, }; pub use self::dml::{ Delete, Insert, InsertOverriding, Merge, MergeAction, MergeClause, MergeClauseKind, diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 014e4f0d32..57ad0df5b5 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -12049,8 +12049,15 @@ impl<'a> Parser<'a> { operation, }) } - Keyword::FUNCTION => self.parse_alter_function(AlterFunctionKind::Function), - Keyword::AGGREGATE => self.parse_alter_function(AlterFunctionKind::Aggregate), + Keyword::FUNCTION => { + let signature = AlterFunctionSignature::Function(self.parse_function_desc()?); + self.parse_alter_function(signature) + } + Keyword::AGGREGATE => { + let signature = + AlterFunctionSignature::Aggregate(self.parse_alter_aggregate_signature()?); + self.parse_alter_function(signature) + } Keyword::OPERATOR => { if self.parse_keyword(Keyword::FAMILY) { self.parse_alter_operator_family().map(Into::into) @@ -12078,22 +12085,16 @@ impl<'a> Parser<'a> { } } - fn parse_alter_aggregate_signature( - &mut self, - ) -> Result<(FunctionDesc, bool, Option>), ParserError> { + fn parse_alter_aggregate_signature(&mut self) -> Result { let name = self.parse_object_name(false)?; self.expect_token(&Token::LParen)?; if self.consume_token(&Token::Mul) { self.expect_token(&Token::RParen)?; - return Ok(( - FunctionDesc { - name, - args: Some(vec![]), - }, - true, - None, - )); + return Ok(AggregateSignature { + name, + args: AggregateArgs::Star, + }); } let args = @@ -12103,21 +12104,17 @@ impl<'a> Parser<'a> { self.parse_comma_separated(Parser::parse_aggregate_function_arg)? }; - let aggregate_order_by = if self.parse_keywords(&[Keyword::ORDER, Keyword::BY]) { + let order_by = if self.parse_keywords(&[Keyword::ORDER, Keyword::BY]) { Some(self.parse_comma_separated(Parser::parse_aggregate_function_arg)?) } else { None }; self.expect_token(&Token::RParen)?; - Ok(( - FunctionDesc { - name, - args: Some(args), - }, - false, - aggregate_order_by, - )) + Ok(AggregateSignature { + name, + args: AggregateArgs::List { args, order_by }, + }) } fn parse_alter_function_action(&mut self) -> Result, ParserError> { @@ -12236,16 +12233,11 @@ impl<'a> Parser<'a> { Ok((actions, restrict)) } - /// Parse an `ALTER FUNCTION` or `ALTER AGGREGATE` statement. + /// Parse an `ALTER FUNCTION` or `ALTER AGGREGATE` operation after its signature. pub fn parse_alter_function( &mut self, - kind: AlterFunctionKind, + signature: AlterFunctionSignature, ) -> Result { - let (function, aggregate_star, aggregate_order_by) = match kind { - AlterFunctionKind::Function => (self.parse_function_desc()?, false, None), - AlterFunctionKind::Aggregate => self.parse_alter_aggregate_signature()?, - }; - let operation = if self.parse_keywords(&[Keyword::RENAME, Keyword::TO]) { let new_name = self.parse_identifier()?; AlterFunctionOperation::RenameTo { new_name } @@ -12255,7 +12247,9 @@ impl<'a> Parser<'a> { AlterFunctionOperation::SetSchema { schema_name: self.parse_object_name(false)?, } - } else if matches!(kind, AlterFunctionKind::Function) && self.parse_keyword(Keyword::NO) { + } else if matches!(signature, AlterFunctionSignature::Function(_)) + && self.parse_keyword(Keyword::NO) + { if !self.parse_keyword(Keyword::DEPENDS) { return self.expected_ref("DEPENDS after NO", self.peek_token_ref()); } @@ -12264,7 +12258,7 @@ impl<'a> Parser<'a> { no: true, extension_name: self.parse_object_name(false)?, } - } else if matches!(kind, AlterFunctionKind::Function) + } else if matches!(signature, AlterFunctionSignature::Function(_)) && self.parse_keyword(Keyword::DEPENDS) { self.expect_keywords(&[Keyword::ON, Keyword::EXTENSION])?; @@ -12272,7 +12266,7 @@ impl<'a> Parser<'a> { no: false, extension_name: self.parse_object_name(false)?, } - } else if matches!(kind, AlterFunctionKind::Function) { + } else if matches!(signature, AlterFunctionSignature::Function(_)) { let (actions, restrict) = self.parse_alter_function_actions()?; AlterFunctionOperation::Actions { actions, restrict } } else { @@ -12283,10 +12277,7 @@ impl<'a> Parser<'a> { }; Ok(Statement::AlterFunction(AlterFunction { - kind, - function, - aggregate_order_by, - aggregate_star, + signature, operation, })) } diff --git a/tests/sqlparser_postgres.rs b/tests/sqlparser_postgres.rs index 923b33daa3..450572bcaa 100644 --- a/tests/sqlparser_postgres.rs +++ b/tests/sqlparser_postgres.rs @@ -9236,7 +9236,7 @@ fn parse_alter_function_and_aggregate() { assert!(matches!( statement, Statement::AlterFunction(AlterFunction { - kind: AlterFunctionKind::Aggregate, + signature: AlterFunctionSignature::Aggregate(_), .. }) )); @@ -9376,7 +9376,7 @@ fn parse_alter_function_and_aggregate() { assert!(matches!( statement, Statement::AlterFunction(AlterFunction { - kind: AlterFunctionKind::Function, + signature: AlterFunctionSignature::Function(_), .. }) )); @@ -10761,23 +10761,29 @@ fn parse_quoted_argument_names_in_function_signatures() { pg_and_generic().verified_stmt(r#"ALTER FUNCTION f("Role" TEXT) RENAME TO g"#); pg_and_generic().verified_stmt(r#"ALTER AGGREGATE my_agg("Role" TEXT) RENAME TO other_agg"#); - // An aggregate's `ORDER BY` arguments are parsed by the same reader as its - // direct ones. + // Aggregate arguments preserve quoted names in both lists. let sql = r#"ALTER AGGREGATE my_agg("A" INT ORDER BY "B" INT) OWNER TO some_role"#; let Statement::AlterFunction(alter) = pg_and_generic().verified_stmt(sql) else { panic!("expected an ALTER AGGREGATE statement"); }; + let AlterFunctionSignature::Aggregate(AggregateSignature { + args: AggregateArgs::List { args, order_by }, + .. + }) = alter.signature + else { + panic!("expected an aggregate argument list"); + }; assert_eq!( - alter.function.args, - Some(vec![OperateFunctionArg { + args, + vec![OperateFunctionArg { mode: None, name: Some(Ident::with_quote('"', "A")), data_type: DataType::Int(None), default_expr: None, - }]) + }] ); assert_eq!( - alter.aggregate_order_by, + order_by, Some(vec![OperateFunctionArg { mode: None, name: Some(Ident::with_quote('"', "B")), @@ -13838,3 +13844,92 @@ fn parse_alter_extension_drop_member() { ); } } + +#[test] +fn display_alter_aggregate_signature_propagates_write_errors() { + for (sql, fragments) in [ + ( + "ALTER AGGREGATE agg(*) RENAME TO other", + vec!["ALTER ", "AGGREGATE ", "agg", "(", "*", ")"], + ), + ( + "ALTER AGGREGATE agg(INT ORDER BY TEXT) RENAME TO other", + vec!["INT", " ", "ORDER BY ", "TEXT"], + ), + ( + "ALTER AGGREGATE agg(ORDER BY TEXT) RENAME TO other", + vec!["ORDER BY ", "TEXT"], + ), + ( + "ALTER FUNCTION fun(INT) RENAME TO other", + vec!["FUNCTION ", "fun"], + ), + ] { + let statement = pg_and_generic().verified_stmt(sql); + assert_display_propagates_write_errors(&statement, &fragments); + } +} + +#[test] +fn parse_alter_aggregate_signatures() { + for (signature, args) in [ + ("agg(*)", AggregateArgs::Star), + ( + "agg()", + AggregateArgs::List { + args: vec![], + order_by: None, + }, + ), + ( + "agg(INT)", + AggregateArgs::List { + args: vec![OperateFunctionArg::unnamed(DataType::Int(None))], + order_by: None, + }, + ), + ( + "agg(ORDER BY TEXT)", + AggregateArgs::List { + args: vec![], + order_by: Some(vec![OperateFunctionArg::unnamed(DataType::Text)]), + }, + ), + ] { + let statement = + pg_and_generic().verified_stmt(&format!("ALTER AGGREGATE {signature} RENAME TO other")); + assert_eq!( + statement, + Statement::AlterFunction(AlterFunction { + signature: AlterFunctionSignature::Aggregate(AggregateSignature { + name: ObjectName::from(Ident::new("agg")), + args, + }), + operation: AlterFunctionOperation::RenameTo { + new_name: Ident::new("other"), + }, + }) + ); + } + + for (sql, error) in [ + ("ALTER FUNCTION 1", "Expected: identifier, found: 1"), + ("ALTER AGGREGATE 1", "Expected: identifier, found: 1"), + ("ALTER AGGREGATE agg", "Expected: (, found: EOF"), + ("ALTER AGGREGATE agg(*", "Expected: ), found: EOF"), + ( + "ALTER AGGREGATE agg(ORDER BY)", + "Expected: a data type name, found: )", + ), + ( + "ALTER FUNCTION fun() NO", + "Expected: DEPENDS after NO, found: EOF", + ), + ] { + assert_eq!( + pg_and_generic().parse_sql_statements(sql), + Err(ParserError::ParserError(error.into())), + "{sql}" + ); + } +}