Address CREATE AGGREGATE review feedback

- Replace em-dashes in CreateAggregateOption doc comments
- Add FunctionParallel::as_str and reuse it in Display and the PARALLEL option
- Support old-syntax BASETYPE option (parse and Display round-trip)
- Clarify the CreateAggregate args field doc
- Test the unknown-option error path and the old BASETYPE syntax

Author: fmguerreiro

src/ast/ddl.rs

@@ -5833,7 +5833,7 @@ pub struct CreateAggregate {
     pub or_replace: bool,
     /// The aggregate name (can be schema-qualified).
     pub name: ObjectName,
-    /// Input arguments. Empty for zero-argument aggregates or `(*)`.
+    /// Input arguments. Empty when `star_args` is true or when the argument list is empty.
     pub args: Vec<OperateFunctionArg>,
     /// True if the argument list was the wildcard form `(*)` (used by
     /// zero-argument aggregates such as `count(*)`).
@@ -5850,10 +5850,16 @@ impl fmt::Display for CreateAggregate {
             write!(f, " OR REPLACE")?;
         }
         write!(f, " AGGREGATE {}", self.name)?;
-        if self.star_args && self.args.is_empty() {
-            write!(f, " (*)")?;
-        } else {
-            write!(f, " ({})", display_comma_separated(&self.args))?;
+        let is_old_syntax = self
+            .options
+            .iter()
+            .any(|option| matches!(option, CreateAggregateOption::BaseType(_)));
+        if !is_old_syntax {
+            if self.star_args && self.args.is_empty() {
+                write!(f, " (*)")?;
+            } else {
+                write!(f, " ({})", display_comma_separated(&self.args))?;
+            }
         }
         write!(f, " ({})", display_comma_separated(&self.options))
     }
@@ -5880,7 +5886,7 @@ pub enum CreateAggregateOption {
     StateDataSize(u64),
     /// `FINALFUNC = final_function`
     FinalFunction(ObjectName),
-    /// `FINALFUNC_EXTRA` — pass extra dummy arguments to the final function.
+    /// `FINALFUNC_EXTRA`. Passes extra dummy arguments to the final function.
     FinalFunctionExtra,
     /// `FINALFUNC_MODIFY = { READ_ONLY | SHAREABLE | READ_WRITE }`
     FinalFunctionModify(AggregateModifyKind),
@@ -5912,8 +5918,10 @@ pub enum CreateAggregateOption {
     SortOperator(ObjectName),
     /// `PARALLEL = { SAFE | RESTRICTED | UNSAFE }`
     Parallel(FunctionParallel),
-    /// `HYPOTHETICAL` — marks the aggregate as hypothetical-set.
+    /// `HYPOTHETICAL`. Marks the aggregate as hypothetical-set.
     Hypothetical,
+    /// `BASETYPE = base_type` (old aggregate syntax).
+    BaseType(DataType),
 }
 
 impl fmt::Display for CreateAggregateOption {
@@ -5938,15 +5946,9 @@ impl fmt::Display for CreateAggregateOption {
             Self::MovingFinalFunctionModify(kind) => write!(f, "MFINALFUNC_MODIFY = {kind}"),
             Self::MovingInitialCondition(cond) => write!(f, "MINITCOND = {cond}"),
             Self::SortOperator(name) => write!(f, "SORTOP = {name}"),
-            Self::Parallel(parallel) => {
-                let kind = match parallel {
-                    FunctionParallel::Safe => "SAFE",
-                    FunctionParallel::Restricted => "RESTRICTED",
-                    FunctionParallel::Unsafe => "UNSAFE",
-                };
-                write!(f, "PARALLEL = {kind}")
-            }
+            Self::Parallel(parallel) => write!(f, "PARALLEL = {}", parallel.as_str()),
             Self::Hypothetical => write!(f, "HYPOTHETICAL"),
+            Self::BaseType(data_type) => write!(f, "BASETYPE = {data_type}"),
         }
     }
 }

src/ast/mod.rs

@@ -10069,16 +10069,23 @@ pub enum FunctionParallel {
     Safe,
 }
 
-impl fmt::Display for FunctionParallel {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+impl FunctionParallel {
+    /// Returns the bare keyword for this parallel mode, without the `PARALLEL` prefix.
+    pub fn as_str(&self) -> &'static str {
         match self {
-            FunctionParallel::Unsafe => write!(f, "PARALLEL UNSAFE"),
-            FunctionParallel::Restricted => write!(f, "PARALLEL RESTRICTED"),
-            FunctionParallel::Safe => write!(f, "PARALLEL SAFE"),
+            FunctionParallel::Unsafe => "UNSAFE",
+            FunctionParallel::Restricted => "RESTRICTED",
+            FunctionParallel::Safe => "SAFE",
         }
     }
 }
 
+impl fmt::Display for FunctionParallel {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "PARALLEL {}", self.as_str())
+    }
+}
+
 /// [BigQuery] Determinism specifier used in a UDF definition.
 ///
 /// [BigQuery]: https://cloud.google.com/bigquery/docs/reference/standard-sql/data-definition-language#syntax_11

src/parser/mod.rs

@@ -7235,22 +7235,31 @@ impl<'a> Parser<'a> {
     ) -> Result<CreateAggregate, ParserError> {
         let name = self.parse_object_name(false)?;
 
-        // Argument list: `(input_arg [, ...])` or `(*)` for zero-arg.
+        // The old syntax has a single parenthesized list that carries both
+        // `BASETYPE` and the remaining options. The modern syntax has a
+        // separate `(input_arg [, ...])` argument list followed by the
+        // options list. They are distinguished by whether a second
+        // parenthesized list follows the first.
+        let checkpoint = self.index;
         self.expect_token(&Token::LParen)?;
-        let star_args = self.consume_token(&Token::Mul);
-        let args = if star_args || self.peek_token().token == Token::RParen {
+        let mut star_args = self.consume_token(&Token::Mul);
+        let mut args = if star_args || self.peek_token().token == Token::RParen {
             vec![]
         } else {
             self.parse_comma_separated(Parser::parse_function_arg)?
         };
         self.expect_token(&Token::RParen)?;
 
-        // Options block: `( SFUNC = ..., STYPE = ..., ... )`.
-        self.expect_token(&Token::LParen)?;
-        let options = self.parse_comma_separated(|parser| {
-            let key = parser.parse_identifier()?;
-            parser.parse_create_aggregate_option(&key.value.to_uppercase())
-        })?;
+        // The first list is the modern argument-type list only when a second
+        // list follows. Otherwise it was the old-syntax options list, so rewind,
+        // discard the speculatively parsed args, and re-parse it as options.
+        if !self.consume_token(&Token::LParen) {
+            self.index = checkpoint;
+            args = vec![];
+            star_args = false;
+            self.expect_token(&Token::LParen)?;
+        }
+        let options = self.parse_comma_separated(Parser::parse_create_aggregate_option_entry)?;
         self.expect_token(&Token::RParen)?;
 
         Ok(CreateAggregate {
@@ -7275,6 +7284,13 @@ impl<'a> Parser<'a> {
         }
     }
 
+    fn parse_create_aggregate_option_entry(
+        &mut self,
+    ) -> Result<CreateAggregateOption, ParserError> {
+        let key = self.parse_identifier()?;
+        self.parse_create_aggregate_option(&key.value.to_uppercase())
+    }
+
     fn parse_create_aggregate_option(
         &mut self,
         key: &str,
@@ -7387,6 +7403,10 @@ impl<'a> Parser<'a> {
                 ))
             }
             "HYPOTHETICAL" => Ok(CreateAggregateOption::Hypothetical),
+            "BASETYPE" => {
+                self.expect_token(&Token::Eq)?;
+                Ok(CreateAggregateOption::BaseType(self.parse_data_type()?))
+            }
             other => Err(ParserError::ParserError(format!(
                 "Unknown CREATE AGGREGATE option: {other}"
             ))),

tests/sqlparser_postgres.rs

@@ -9348,3 +9348,43 @@ fn parse_create_aggregate_additional_options() {
         "CREATE AGGREGATE my_sum (INT) (SFUNC = my_sfunc, STYPE = internal, COMBINEFUNC = my_combine, SERIALFUNC = my_serial, DESERIALFUNC = my_deserial)",
     );
 }
+
+#[test]
+fn parse_create_aggregate_old_syntax_basetype() {
+    let stmt = pg_and_generic()
+        .verified_stmt("CREATE AGGREGATE my_avg (BASETYPE = INT, SFUNC = my_sfunc, STYPE = INT)");
+    match stmt {
+        Statement::CreateAggregate(agg) => {
+            assert!(agg.args.is_empty());
+            assert!(!agg.star_args);
+            assert_eq!(agg.options.len(), 3);
+            assert_eq!(
+                agg.options[0],
+                CreateAggregateOption::BaseType(DataType::Int(None))
+            );
+        }
+        _ => panic!("Expected CreateAggregate, got: {stmt:?}"),
+    }
+
+    let stmt = pg_and_generic()
+        .verified_stmt("CREATE AGGREGATE my_avg (SFUNC = my_sfunc, BASETYPE = INT, STYPE = INT)");
+    match stmt {
+        Statement::CreateAggregate(agg) => {
+            assert!(agg.args.is_empty());
+            assert!(!agg.star_args);
+            assert_eq!(agg.options.len(), 3);
+            assert_eq!(
+                agg.options[1],
+                CreateAggregateOption::BaseType(DataType::Int(None))
+            );
+        }
+        _ => panic!("Expected CreateAggregate, got: {stmt:?}"),
+    }
+}
+
+#[test]
+fn parse_create_aggregate_unknown_option() {
+    assert!(pg_and_generic()
+        .parse_sql_statements("CREATE AGGREGATE foo (INT) (UNKNOWN_OPTION = bar)")
+        .is_err());
+}