Snowflake EXTERNAL VOLUME: reuse KeyValueOptions and generic DROP

Address review feedback by building on existing infrastructure instead of
bespoke AST:

- Parse storage locations (and the nested ENCRYPTION clause) via
  Parser::parse_key_value_options, the same path CREATE STAGE uses. Storage
  locations are now Vec<KeyValueOptions>; the ExternalVolumeStorageLocation
  and ExternalVolumeEncryption structs and their hand-written Display and
  parser code are removed. Parsing is syntax-only, so field order is
  preserved and semantic checks (required STORAGE_BASE_URL, duplicate
  fields) are left to the consumer.
- Route DROP EXTERNAL VOLUME through the generic Statement::Drop machinery
  via a new ObjectType::ExternalVolume, matching how STAGE/STREAM are
  handled, and drop the dedicated DropExternalVolume statement variant.
- Use expect_keyword_is instead of expect_keyword to match the file's
  convention, and remove the now-unused option-name keywords.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: sabir-akhadov-localstack

src/ast/mod.rs

@@ -4546,8 +4546,9 @@ pub enum Statement {
         if_not_exists: bool,
         /// External volume name.
         name: ObjectName,
-        /// Storage locations.
-        storage_locations: Vec<ExternalVolumeStorageLocation>,
+        /// Storage locations, each a parenthesized list of key-value options
+        /// (e.g. `(NAME='loc1' STORAGE_PROVIDER='S3' STORAGE_BASE_URL='s3://bucket/')`).
+        storage_locations: Vec<KeyValueOptions>,
         /// Optional `ALLOW_WRITES` setting.
         allow_writes: Option<bool>,
         /// Optional comment.
@@ -4565,15 +4566,6 @@ pub enum Statement {
         operation: AlterExternalVolumeOperation,
     },
     /// ```sql
-    /// DROP EXTERNAL VOLUME [IF EXISTS] <name>
-    /// ```
-    DropExternalVolume {
-        /// External volume name.
-        name: ObjectName,
-        /// `IF EXISTS` flag.
-        if_exists: bool,
-    },
-    /// ```sql
     /// DESC[RIBE] EXTERNAL VOLUME <name>
     /// ```
     DescribeExternalVolume {
@@ -6353,13 +6345,6 @@ impl fmt::Display for Statement {
                     if_exists = if *if_exists { "IF EXISTS " } else { "" },
                 )
             }
-            Statement::DropExternalVolume { name, if_exists } => {
-                write!(
-                    f,
-                    "DROP EXTERNAL VOLUME {if_exists}{name}",
-                    if_exists = if *if_exists { "IF EXISTS " } else { "" },
-                )
-            }
             Statement::DescribeExternalVolume { name } => {
                 write!(f, "DESCRIBE EXTERNAL VOLUME {name}")
             }
@@ -8688,6 +8673,8 @@ pub enum ObjectType {
     User,
     /// A stream.
     Stream,
+    /// A Snowflake external volume.
+    ExternalVolume,
 }
 
 impl fmt::Display for ObjectType {
@@ -8706,6 +8693,7 @@ impl fmt::Display for ObjectType {
             ObjectType::Type => "TYPE",
             ObjectType::User => "USER",
             ObjectType::Stream => "STREAM",
+            ObjectType::ExternalVolume => "EXTERNAL VOLUME",
         })
     }
 }
@@ -11130,91 +11118,13 @@ pub struct ShowObjects {
     pub show_options: ShowStatementOptions,
 }
 
-/// A storage location within a Snowflake `EXTERNAL VOLUME`.
-#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
-#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
-#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
-pub struct ExternalVolumeStorageLocation {
-    /// The `NAME` of the storage location.
-    pub name: String,
-    /// The `STORAGE_PROVIDER` (e.g. `'S3'`).
-    pub storage_provider: String,
-    /// The `STORAGE_BASE_URL` (e.g. `'s3://bucket/path/'`).
-    pub storage_base_url: String,
-    /// Optional `STORAGE_AWS_ROLE_ARN`.
-    pub storage_aws_role_arn: Option<String>,
-    /// Optional `STORAGE_AWS_EXTERNAL_ID`.
-    pub storage_aws_external_id: Option<String>,
-    /// Optional `ENCRYPTION` settings.
-    pub encryption: Option<ExternalVolumeEncryption>,
-}
-
-impl fmt::Display for ExternalVolumeStorageLocation {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(
-            f,
-            "NAME = '{}' STORAGE_PROVIDER = '{}' STORAGE_BASE_URL = '{}'",
-            value::escape_single_quote_string(&self.name),
-            value::escape_single_quote_string(&self.storage_provider),
-            value::escape_single_quote_string(&self.storage_base_url),
-        )?;
-        if let Some(ref arn) = self.storage_aws_role_arn {
-            write!(
-                f,
-                " STORAGE_AWS_ROLE_ARN = '{}'",
-                value::escape_single_quote_string(arn)
-            )?;
-        }
-        if let Some(ref ext_id) = self.storage_aws_external_id {
-            write!(
-                f,
-                " STORAGE_AWS_EXTERNAL_ID = '{}'",
-                value::escape_single_quote_string(ext_id)
-            )?;
-        }
-        if let Some(ref enc) = self.encryption {
-            write!(f, " ENCRYPTION = ({enc})")?;
-        }
-        Ok(())
-    }
-}
-
-/// Encryption settings for an `EXTERNAL VOLUME` storage location.
-#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
-#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
-#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
-pub struct ExternalVolumeEncryption {
-    /// Encryption type: `'AWS_SSE_S3'`, `'AWS_SSE_KMS'`, or `'NONE'`.
-    pub kind: String,
-    /// Optional `KMS_KEY_ID`.
-    pub kms_key_id: Option<String>,
-}
-
-impl fmt::Display for ExternalVolumeEncryption {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(
-            f,
-            "TYPE = '{}'",
-            value::escape_single_quote_string(&self.kind)
-        )?;
-        if let Some(ref key_id) = self.kms_key_id {
-            write!(
-                f,
-                " KMS_KEY_ID = '{}'",
-                value::escape_single_quote_string(key_id)
-            )?;
-        }
-        Ok(())
-    }
-}
-
 /// Operations for `ALTER EXTERNAL VOLUME`.
 #[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
 pub enum AlterExternalVolumeOperation {
     /// `ADD STORAGE_LOCATION = ( ... )`
-    AddStorageLocation(ExternalVolumeStorageLocation),
+    AddStorageLocation(KeyValueOptions),
     /// `SET ALLOW_WRITES = TRUE|FALSE`
     SetAllowWrites(bool),
     /// `REMOVE STORAGE_LOCATION '<name>'`

src/ast/spans.rs

@@ -524,7 +524,6 @@ impl Spanned for Statement {
             Statement::Reset(..) => Span::empty(),
             Statement::CreateExternalVolume { .. } => Span::empty(),
             Statement::AlterExternalVolume { .. } => Span::empty(),
-            Statement::DropExternalVolume { .. } => Span::empty(),
             Statement::DescribeExternalVolume { .. } => Span::empty(),
             Statement::ShowExternalVolumes { .. } => Span::empty(),
         }

src/dialect/snowflake.rs

@@ -29,14 +29,13 @@ use crate::ast::helpers::stmt_data_loading::{
 use crate::ast::{
     AlterExternalVolumeOperation, AlterTable, AlterTableOperation, AlterTableType,
     CatalogSyncNamespaceMode, ColumnOption, ColumnPolicy, ColumnPolicyProperty, ContactEntry,
-    CopyIntoSnowflakeKind, CreateTable, CreateTableLikeKind, DollarQuotedString,
-    ExternalVolumeEncryption, ExternalVolumeStorageLocation, Ident, IdentityParameters,
-    IdentityProperty, IdentityPropertyFormatKind, IdentityPropertyKind, IdentityPropertyOrder,
-    InitializeKind, Insert, MultiTableInsertIntoClause, MultiTableInsertType,
-    MultiTableInsertValue, MultiTableInsertValues, MultiTableInsertWhenClause, ObjectName,
-    ObjectNamePart, RefreshModeKind, RowAccessPolicy, ShowObjects, SqlOption, Statement,
-    StorageLifecyclePolicy, StorageSerializationPolicy, TableObject, TagsColumnOption, Value,
-    WrappedCollection,
+    CopyIntoSnowflakeKind, CreateTable, CreateTableLikeKind, DollarQuotedString, Ident,
+    IdentityParameters, IdentityProperty, IdentityPropertyFormatKind, IdentityPropertyKind,
+    IdentityPropertyOrder, InitializeKind, Insert, MultiTableInsertIntoClause,
+    MultiTableInsertType, MultiTableInsertValue, MultiTableInsertValues,
+    MultiTableInsertWhenClause, ObjectName, ObjectNamePart, RefreshModeKind, RowAccessPolicy,
+    ShowObjects, SqlOption, Statement, StorageLifecyclePolicy, StorageSerializationPolicy,
+    TableObject, TagsColumnOption, Value, WrappedCollection,
 };
 use crate::dialect::{Dialect, Precedence};
 use crate::keywords::Keyword;
@@ -288,11 +287,6 @@ impl Dialect for SnowflakeDialect {
             return Some(parse_alter_session(parser, set));
         }
 
-        if parser.parse_keywords(&[Keyword::DROP, Keyword::EXTERNAL, Keyword::VOLUME]) {
-            // DROP EXTERNAL VOLUME
-            return Some(parse_drop_external_volume(parser));
-        }
-
         if parser
             .parse_one_of_keywords(&[Keyword::DESC, Keyword::DESCRIBE])
             .is_some()
@@ -2018,22 +2012,25 @@ fn parse_multi_table_insert_when_clauses(
 }
 
 /// Parse `CREATE [OR REPLACE] EXTERNAL VOLUME [IF NOT EXISTS] <name> ...`
+///
+/// Each storage location is a parenthesized `KEY = value` option list; the
+/// options (and the `ENCRYPTION = (...)` sub-list) are parsed generically via
+/// [`Parser::parse_key_value_options`], so field order and the exact option
+/// set are left to the caller rather than validated here.
 fn parse_create_external_volume(
     or_replace: bool,
     parser: &mut Parser,
 ) -> Result<Statement, ParserError> {
     let if_not_exists = parser.parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]);
     let name = parser.parse_object_name(false)?;
 
-    parser.expect_keyword(Keyword::STORAGE_LOCATIONS)?;
+    parser.expect_keyword_is(Keyword::STORAGE_LOCATIONS)?;
     parser.expect_token(&Token::Eq)?;
     parser.expect_token(&Token::LParen)?;
 
     let mut storage_locations = vec![];
     loop {
-        parser.expect_token(&Token::LParen)?;
-        storage_locations.push(parse_external_volume_storage_location(parser)?);
-        parser.expect_token(&Token::RParen)?;
+        storage_locations.push(parser.parse_key_value_options(true, &[])?);
         if !parser.consume_token(&Token::Comma) {
             break;
         }
@@ -2063,126 +2060,21 @@ fn parse_create_external_volume(
     })
 }
 
-/// Parse a single storage location: `NAME = '...' STORAGE_PROVIDER = '...' ...`
-///
-/// `NAME` and `STORAGE_PROVIDER` must appear first (in that order). After
-/// those two, `STORAGE_BASE_URL` (required) and the optional fields
-/// (`STORAGE_AWS_ROLE_ARN`, `STORAGE_AWS_EXTERNAL_ID`, `ENCRYPTION`) may
-/// appear in any order, separated by commas or whitespace. Real Snowflake
-/// accepts the flexible ordering — mirror that here so fixtures like
-/// `NAME = '…' STORAGE_PROVIDER = '…' STORAGE_AWS_ROLE_ARN = '…'
-///  STORAGE_BASE_URL = '…'` parse correctly.
-fn parse_external_volume_storage_location(
-    parser: &mut Parser,
-) -> Result<ExternalVolumeStorageLocation, ParserError> {
-    parser.expect_keyword(Keyword::NAME)?;
-    parser.expect_token(&Token::Eq)?;
-    let name = parser.parse_literal_string()?;
-
-    // separators may be commas or whitespace — consume optional comma
-    let _ = parser.consume_token(&Token::Comma);
-
-    parser.expect_keyword(Keyword::STORAGE_PROVIDER)?;
-    parser.expect_token(&Token::Eq)?;
-    let storage_provider = parser.parse_literal_string()?;
-
-    let mut storage_base_url: Option<String> = None;
-    let mut storage_aws_role_arn = None;
-    let mut storage_aws_external_id = None;
-    let mut encryption = None;
-
-    // Parse the remaining fields (STORAGE_BASE_URL + optionals) in any order
-    // (comma-or-whitespace separated). Duplicates are rejected.
-    loop {
-        let _ = parser.consume_token(&Token::Comma);
-        if parser.parse_keyword(Keyword::STORAGE_BASE_URL) {
-            if storage_base_url.is_some() {
-                return Err(ParserError::ParserError(
-                    "duplicate STORAGE_BASE_URL in STORAGE_LOCATION".to_string(),
-                ));
-            }
-            parser.expect_token(&Token::Eq)?;
-            storage_base_url = Some(parser.parse_literal_string()?);
-        } else if parser.parse_keyword(Keyword::STORAGE_AWS_ROLE_ARN) {
-            if storage_aws_role_arn.is_some() {
-                return Err(ParserError::ParserError(
-                    "duplicate STORAGE_AWS_ROLE_ARN in STORAGE_LOCATION".to_string(),
-                ));
-            }
-            parser.expect_token(&Token::Eq)?;
-            storage_aws_role_arn = Some(parser.parse_literal_string()?);
-        } else if parser.parse_keyword(Keyword::STORAGE_AWS_EXTERNAL_ID) {
-            if storage_aws_external_id.is_some() {
-                return Err(ParserError::ParserError(
-                    "duplicate STORAGE_AWS_EXTERNAL_ID in STORAGE_LOCATION".to_string(),
-                ));
-            }
-            parser.expect_token(&Token::Eq)?;
-            storage_aws_external_id = Some(parser.parse_literal_string()?);
-        } else if parser.parse_keyword(Keyword::ENCRYPTION) {
-            if encryption.is_some() {
-                return Err(ParserError::ParserError(
-                    "duplicate ENCRYPTION in STORAGE_LOCATION".to_string(),
-                ));
-            }
-            parser.expect_token(&Token::Eq)?;
-            parser.expect_token(&Token::LParen)?;
-            encryption = Some(parse_external_volume_encryption(parser)?);
-            parser.expect_token(&Token::RParen)?;
-        } else {
-            break;
-        }
-    }
-
-    let storage_base_url = storage_base_url.ok_or_else(|| {
-        ParserError::ParserError("STORAGE_BASE_URL is required in STORAGE_LOCATION".to_string())
-    })?;
-
-    Ok(ExternalVolumeStorageLocation {
-        name,
-        storage_provider,
-        storage_base_url,
-        storage_aws_role_arn,
-        storage_aws_external_id,
-        encryption,
-    })
-}
-
-/// Parse encryption options: `TYPE = '...' [KMS_KEY_ID = '...']`
-fn parse_external_volume_encryption(
-    parser: &mut Parser,
-) -> Result<ExternalVolumeEncryption, ParserError> {
-    parser.expect_keyword(Keyword::TYPE)?;
-    parser.expect_token(&Token::Eq)?;
-    let kind = parser.parse_literal_string()?;
-
-    let mut kms_key_id = None;
-    if parser.parse_keyword(Keyword::KMS_KEY_ID) {
-        parser.expect_token(&Token::Eq)?;
-        kms_key_id = Some(parser.parse_literal_string()?);
-    }
-
-    Ok(ExternalVolumeEncryption { kind, kms_key_id })
-}
-
 /// Parse `ALTER EXTERNAL VOLUME [IF EXISTS] <name> ...`
 fn parse_alter_external_volume(parser: &mut Parser) -> Result<Statement, ParserError> {
     let if_exists = parser.parse_keywords(&[Keyword::IF, Keyword::EXISTS]);
     let name = parser.parse_object_name(false)?;
 
     let operation = if parser.parse_keyword(Keyword::ADD) {
-        parser.expect_keyword(Keyword::STORAGE_LOCATION)?;
+        parser.expect_keyword_is(Keyword::STORAGE_LOCATION)?;
         parser.expect_token(&Token::Eq)?;
-        parser.expect_token(&Token::LParen)?;
-        let loc = parse_external_volume_storage_location(parser)?;
-        parser.expect_token(&Token::RParen)?;
-        AlterExternalVolumeOperation::AddStorageLocation(loc)
+        AlterExternalVolumeOperation::AddStorageLocation(parser.parse_key_value_options(true, &[])?)
     } else if parser.parse_keyword(Keyword::SET) {
-        parser.expect_keyword(Keyword::ALLOW_WRITES)?;
+        parser.expect_keyword_is(Keyword::ALLOW_WRITES)?;
         parser.expect_token(&Token::Eq)?;
         AlterExternalVolumeOperation::SetAllowWrites(parser.parse_boolean_string()?)
     } else if parser.parse_keyword(Keyword::REMOVE) {
-        parser.expect_keyword(Keyword::STORAGE_LOCATION)?;
+        parser.expect_keyword_is(Keyword::STORAGE_LOCATION)?;
         let loc_name = parser.parse_literal_string()?;
         AlterExternalVolumeOperation::RemoveStorageLocation(loc_name)
     } else {
@@ -2199,13 +2091,6 @@ fn parse_alter_external_volume(parser: &mut Parser) -> Result<Statement, ParserE
     })
 }
 
-/// Parse `DROP EXTERNAL VOLUME [IF EXISTS] <name>`
-fn parse_drop_external_volume(parser: &mut Parser) -> Result<Statement, ParserError> {
-    let if_exists = parser.parse_keywords(&[Keyword::IF, Keyword::EXISTS]);
-    let name = parser.parse_object_name(false)?;
-    Ok(Statement::DropExternalVolume { name, if_exists })
-}
-
 /// Parse `DESC[RIBE] EXTERNAL VOLUME <name>`
 fn parse_describe_external_volume(parser: &mut Parser) -> Result<Statement, ParserError> {
     let name = parser.parse_object_name(false)?;

src/keywords.rs

@@ -570,7 +570,6 @@ define_keywords!(
     KEYS,
     KEY_BLOCK_SIZE,
     KILL,
-    KMS_KEY_ID,
     LAG,
     LAMBDA,
     LANGUAGE,
@@ -1006,13 +1005,9 @@ define_keywords!(
     STDOUT,
     STEP,
     STORAGE,
-    STORAGE_AWS_EXTERNAL_ID,
-    STORAGE_AWS_ROLE_ARN,
-    STORAGE_BASE_URL,
     STORAGE_INTEGRATION,
     STORAGE_LOCATION,
     STORAGE_LOCATIONS,
-    STORAGE_PROVIDER,
     STORAGE_SERIALIZATION_POLICY,
     STORED,
     STRAIGHT_JOIN,