From 9618cdaa3d2a629cc948630f10b08ab22a6619d5 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Sun, 19 Nov 2023 13:38:37 +0000 Subject: [PATCH 1/6] Add support for generated virtual columns with expression --- src/ast/ddl.rs | 32 +++++++++++++++++++------------- src/parser/mod.rs | 13 +++++++++++-- tests/sqlparser_sqlite.rs | 6 ++++++ 3 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/ast/ddl.rs b/src/ast/ddl.rs index da2c8c9e4..23df593c0 100644 --- a/src/ast/ddl.rs +++ b/src/ast/ddl.rs @@ -641,20 +641,26 @@ impl fmt::Display for ColumnOption { generation_expr, } => match generated_as { GeneratedAs::Always => { - write!(f, "GENERATED ALWAYS AS IDENTITY")?; - if sequence_options.is_some() { - let so = sequence_options.as_ref().unwrap(); - if !so.is_empty() { - write!(f, " (")?; - } - for sequence_option in so { - write!(f, "{sequence_option}")?; - } - if !so.is_empty() { - write!(f, " )")?; + if let Some(expr) = generation_expr { + // Like SQLite, MySQL - expr evaluated on read + write!(f, "GENERATED ALWAYS AS ({expr}) VIRTUAL") + } else { + // Like Postgres - generated from sequence + write!(f, "GENERATED ALWAYS AS IDENTITY")?; + if sequence_options.is_some() { + let so = sequence_options.as_ref().unwrap(); + if !so.is_empty() { + write!(f, " (")?; + } + for sequence_option in so { + write!(f, "{sequence_option}")?; + } + if !so.is_empty() { + write!(f, " )")?; + } } + Ok(()) } - Ok(()) } GeneratedAs::ByDefault => { write!(f, "GENERATED BY DEFAULT AS IDENTITY")?; @@ -682,7 +688,7 @@ impl fmt::Display for ColumnOption { } /// `GeneratedAs`s are modifiers that follow a column option in a `generated`. -/// 'ExpStored' is PostgreSQL specific +/// 'ExpStored' is used for a column generated from an expression and stored. #[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] diff --git a/src/parser/mod.rs b/src/parser/mod.rs index bad0470c1..7af035fb5 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -4252,9 +4252,18 @@ impl<'a> Parser<'a> { if self.expect_token(&Token::LParen).is_ok() { let expr = self.parse_expr()?; self.expect_token(&Token::RParen)?; - let _ = self.parse_keywords(&[Keyword::STORED]); + let gen_as = if self.parse_keywords(&[Keyword::STORED]) { + Ok(GeneratedAs::ExpStored) + } else if dialect_of!(self is PostgreSqlDialect) { + // Postgres' AS IDENTITY branches are above, this one needs STORED + self.expected("STORED", self.peek_token()) + } else { + let _ = self.parse_keywords(&[Keyword::VIRTUAL]); + Ok(GeneratedAs::Always) + }?; + Ok(Some(ColumnOption::Generated { - generated_as: GeneratedAs::ExpStored, + generated_as: gen_as, sequence_options: None, generation_expr: Some(expr), })) diff --git a/tests/sqlparser_sqlite.rs b/tests/sqlparser_sqlite.rs index 7cfe9422a..019f69c76 100644 --- a/tests/sqlparser_sqlite.rs +++ b/tests/sqlparser_sqlite.rs @@ -205,6 +205,12 @@ fn parse_create_sqlite_quote() { } } +#[test] +fn parse_create_table_gencol_virtual() { + let sql = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) VIRTUAL)"; + sqlite_and_generic().verified_stmt(sql); +} + #[test] fn test_placeholder() { // In postgres, this would be the absolute value operator '@' applied to the column 'xxx' From 05ac5bc0fe35bbbdd0e6be6ed84dfbafc588aae1 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Mon, 20 Nov 2023 10:07:43 +0000 Subject: [PATCH 2/6] Extend generated column tests for SQLite & add for MySQL --- tests/sqlparser_mysql.rs | 11 +++++++++++ tests/sqlparser_sqlite.rs | 11 ++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/sqlparser_mysql.rs b/tests/sqlparser_mysql.rs index c80003b7d..62183b26a 100644 --- a/tests/sqlparser_mysql.rs +++ b/tests/sqlparser_mysql.rs @@ -507,6 +507,17 @@ fn parse_create_table_comment_character_set() { } } +#[test] +fn parse_create_table_gencol() { + let sql_default = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2))"; + let sql_virt = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) VIRTUAL)"; + mysql_and_generic().verified_stmt(sql_virt); + mysql_and_generic().one_statement_parses_to(sql_default, sql_virt); + + let sql_stored = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) STORED)"; + mysql_and_generic().verified_stmt(sql_stored); +} + #[test] fn parse_quote_identifiers() { let sql = "CREATE TABLE `PRIMARY` (`BEGIN` INT PRIMARY KEY)"; diff --git a/tests/sqlparser_sqlite.rs b/tests/sqlparser_sqlite.rs index 019f69c76..2e07a474f 100644 --- a/tests/sqlparser_sqlite.rs +++ b/tests/sqlparser_sqlite.rs @@ -206,9 +206,14 @@ fn parse_create_sqlite_quote() { } #[test] -fn parse_create_table_gencol_virtual() { - let sql = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) VIRTUAL)"; - sqlite_and_generic().verified_stmt(sql); +fn parse_create_table_gencol() { + let sql_default = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2))"; + let sql_virt = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) VIRTUAL)"; + sqlite_and_generic().verified_stmt(sql_virt); + sqlite_and_generic().one_statement_parses_to(sql_default, sql_virt); + + let sql_stored = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) STORED)"; + sqlite_and_generic().verified_stmt(sql_stored); } #[test] From ec986b0cb733f7b72290f34b60db5948a0a746bf Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Mon, 20 Nov 2023 10:08:06 +0000 Subject: [PATCH 3/6] Remove outdated comment --- tests/sqlparser_sqlite.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/sqlparser_sqlite.rs b/tests/sqlparser_sqlite.rs index 2e07a474f..e4663c71e 100644 --- a/tests/sqlparser_sqlite.rs +++ b/tests/sqlparser_sqlite.rs @@ -446,7 +446,6 @@ fn sqlite_with_options(options: ParserOptions) -> TestedDialects { fn sqlite_and_generic() -> TestedDialects { TestedDialects { - // we don't have a separate SQLite dialect, so test only the generic dialect for now dialects: vec![Box::new(SQLiteDialect {}), Box::new(GenericDialect {})], options: None, } From f0d0f5b52cf6bf5f4240c25e8bbb8359beb3a45f Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Tue, 21 Nov 2023 08:42:33 +0000 Subject: [PATCH 4/6] Remove redundant check in tests --- tests/sqlparser_mysql.rs | 1 - tests/sqlparser_sqlite.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/tests/sqlparser_mysql.rs b/tests/sqlparser_mysql.rs index 62183b26a..e275bdb4a 100644 --- a/tests/sqlparser_mysql.rs +++ b/tests/sqlparser_mysql.rs @@ -511,7 +511,6 @@ fn parse_create_table_comment_character_set() { fn parse_create_table_gencol() { let sql_default = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2))"; let sql_virt = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) VIRTUAL)"; - mysql_and_generic().verified_stmt(sql_virt); mysql_and_generic().one_statement_parses_to(sql_default, sql_virt); let sql_stored = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) STORED)"; diff --git a/tests/sqlparser_sqlite.rs b/tests/sqlparser_sqlite.rs index e4663c71e..f15bc0dce 100644 --- a/tests/sqlparser_sqlite.rs +++ b/tests/sqlparser_sqlite.rs @@ -209,7 +209,6 @@ fn parse_create_sqlite_quote() { fn parse_create_table_gencol() { let sql_default = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2))"; let sql_virt = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) VIRTUAL)"; - sqlite_and_generic().verified_stmt(sql_virt); sqlite_and_generic().one_statement_parses_to(sql_default, sql_virt); let sql_stored = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) STORED)"; From 6d69a430343616cce39699944748005765f56401 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 22 Nov 2023 16:10:11 +0000 Subject: [PATCH 5/6] Preserve presence/absence of VIRTUAL for generated columns --- src/ast/ddl.rs | 61 ++++++++++++++++++++------------------- src/ast/mod.rs | 4 +-- src/parser/mod.rs | 15 +++++++--- src/test_utils.rs | 2 ++ tests/sqlparser_mysql.rs | 5 +++- tests/sqlparser_sqlite.rs | 4 ++- 6 files changed, 53 insertions(+), 38 deletions(-) diff --git a/src/ast/ddl.rs b/src/ast/ddl.rs index 23df593c0..3192af8bb 100644 --- a/src/ast/ddl.rs +++ b/src/ast/ddl.rs @@ -599,6 +599,7 @@ pub enum ColumnOption { generated_as: GeneratedAs, sequence_options: Option>, generation_expr: Option, + generation_expr_mode: Option, }, } @@ -639,31 +640,25 @@ impl fmt::Display for ColumnOption { generated_as, sequence_options, generation_expr, - } => match generated_as { - GeneratedAs::Always => { - if let Some(expr) = generation_expr { - // Like SQLite, MySQL - expr evaluated on read - write!(f, "GENERATED ALWAYS AS ({expr}) VIRTUAL") - } else { - // Like Postgres - generated from sequence - write!(f, "GENERATED ALWAYS AS IDENTITY")?; - if sequence_options.is_some() { - let so = sequence_options.as_ref().unwrap(); - if !so.is_empty() { - write!(f, " (")?; - } - for sequence_option in so { - write!(f, "{sequence_option}")?; - } - if !so.is_empty() { - write!(f, " )")?; - } - } - Ok(()) - } - } - GeneratedAs::ByDefault => { - write!(f, "GENERATED BY DEFAULT AS IDENTITY")?; + generation_expr_mode, + } => { + if let Some(expr) = generation_expr { + let modifier = match generation_expr_mode { + None => "", + Some(GeneratedExpressionMode::Virtual) => " VIRTUAL", + Some(GeneratedExpressionMode::Stored) => " STORED", + }; + write!(f, "GENERATED ALWAYS AS ({expr}){modifier}")?; + Ok(()) + } else { + // Like Postgres - generated from sequence + let when = match generated_as { + GeneratedAs::Always => "ALWAYS", + GeneratedAs::ByDefault => "BY DEFAULT", + // ExpStored goes with an expression, handled above + GeneratedAs::ExpStored => unreachable!(), + }; + write!(f, "GENERATED {when} AS IDENTITY")?; if sequence_options.is_some() { let so = sequence_options.as_ref().unwrap(); if !so.is_empty() { @@ -678,11 +673,7 @@ impl fmt::Display for ColumnOption { } Ok(()) } - GeneratedAs::ExpStored => { - let expr = generation_expr.as_ref().unwrap(); - write!(f, "GENERATED ALWAYS AS ({expr}) STORED") - } - }, + } } } } @@ -698,6 +689,16 @@ pub enum GeneratedAs { ExpStored, } +/// `GeneratedExpressionMode`s are modifiers that follow an expression in a `generated`. +/// No modifier is typically the same as Virtual. +#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] +#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] +#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] +pub enum GeneratedExpressionMode { + Virtual, + Stored, +} + fn display_constraint_name(name: &'_ Option) -> impl fmt::Display + '_ { struct ConstraintName<'a>(&'a Option); impl<'a> fmt::Display for ConstraintName<'a> { diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 3929d228b..bcee183f6 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -31,8 +31,8 @@ pub use self::data_type::{ pub use self::dcl::{AlterRoleOperation, ResetConfig, RoleOption, SetConfigValue}; pub use self::ddl::{ AlterColumnOperation, AlterIndexOperation, AlterTableOperation, ColumnDef, ColumnOption, - ColumnOptionDef, GeneratedAs, IndexType, KeyOrIndexDisplay, Partition, ProcedureParam, - ReferentialAction, TableConstraint, UserDefinedTypeCompositeAttributeDef, + ColumnOptionDef, GeneratedAs, GeneratedExpressionMode, IndexType, KeyOrIndexDisplay, Partition, + ProcedureParam, ReferentialAction, TableConstraint, UserDefinedTypeCompositeAttributeDef, UserDefinedTypeRepresentation, }; pub use self::operator::{BinaryOperator, UnaryOperator}; diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 7af035fb5..079a25847 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -4231,6 +4231,7 @@ impl<'a> Parser<'a> { generated_as: GeneratedAs::Always, sequence_options: Some(sequence_options), generation_expr: None, + generation_expr_mode: None, })) } else if self.parse_keywords(&[ Keyword::BY, @@ -4247,25 +4248,31 @@ impl<'a> Parser<'a> { generated_as: GeneratedAs::ByDefault, sequence_options: Some(sequence_options), generation_expr: None, + generation_expr_mode: None, })) } else if self.parse_keywords(&[Keyword::ALWAYS, Keyword::AS]) { if self.expect_token(&Token::LParen).is_ok() { let expr = self.parse_expr()?; self.expect_token(&Token::RParen)?; - let gen_as = if self.parse_keywords(&[Keyword::STORED]) { - Ok(GeneratedAs::ExpStored) + let (gen_as, expr_mode) = if self.parse_keywords(&[Keyword::STORED]) { + Ok(( + GeneratedAs::ExpStored, + Some(GeneratedExpressionMode::Stored), + )) } else if dialect_of!(self is PostgreSqlDialect) { // Postgres' AS IDENTITY branches are above, this one needs STORED self.expected("STORED", self.peek_token()) + } else if self.parse_keywords(&[Keyword::VIRTUAL]) { + Ok((GeneratedAs::Always, Some(GeneratedExpressionMode::Virtual))) } else { - let _ = self.parse_keywords(&[Keyword::VIRTUAL]); - Ok(GeneratedAs::Always) + Ok((GeneratedAs::Always, None)) }?; Ok(Some(ColumnOption::Generated { generated_as: gen_as, sequence_options: None, generation_expr: Some(expr), + generation_expr_mode: expr_mode, })) } else { Ok(None) diff --git a/src/test_utils.rs b/src/test_utils.rs index 76a3e073b..d8216d51a 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -120,6 +120,8 @@ impl TestedDialects { let only_statement = statements.pop().unwrap(); if !canonical.is_empty() { + println!("Canonical: {canonical}"); + println!("Reformed: {only_statement}"); assert_eq!(canonical, only_statement.to_string()) } only_statement diff --git a/tests/sqlparser_mysql.rs b/tests/sqlparser_mysql.rs index e275bdb4a..b2b089e60 100644 --- a/tests/sqlparser_mysql.rs +++ b/tests/sqlparser_mysql.rs @@ -15,6 +15,7 @@ //! is also tested (on the inputs it can handle). use matches::assert_matches; +use sqlparser; use sqlparser::ast::Expr; use sqlparser::ast::Value; use sqlparser::ast::*; @@ -510,8 +511,10 @@ fn parse_create_table_comment_character_set() { #[test] fn parse_create_table_gencol() { let sql_default = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2))"; + mysql_and_generic().verified_stmt(sql_default); + let sql_virt = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) VIRTUAL)"; - mysql_and_generic().one_statement_parses_to(sql_default, sql_virt); + mysql_and_generic().verified_stmt(sql_virt); let sql_stored = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) STORED)"; mysql_and_generic().verified_stmt(sql_stored); diff --git a/tests/sqlparser_sqlite.rs b/tests/sqlparser_sqlite.rs index f15bc0dce..cc0d53b14 100644 --- a/tests/sqlparser_sqlite.rs +++ b/tests/sqlparser_sqlite.rs @@ -208,8 +208,10 @@ fn parse_create_sqlite_quote() { #[test] fn parse_create_table_gencol() { let sql_default = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2))"; + sqlite_and_generic().verified_stmt(sql_default); + let sql_virt = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) VIRTUAL)"; - sqlite_and_generic().one_statement_parses_to(sql_default, sql_virt); + sqlite_and_generic().verified_stmt(sql_virt); let sql_stored = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) STORED)"; sqlite_and_generic().verified_stmt(sql_stored); From 2887501f36c49b1ee0ef4ccde5ee8aac4f53a634 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 22 Nov 2023 18:07:11 +0000 Subject: [PATCH 6/6] Remove some leftover debugging --- src/test_utils.rs | 2 -- tests/sqlparser_mysql.rs | 1 - 2 files changed, 3 deletions(-) diff --git a/src/test_utils.rs b/src/test_utils.rs index d8216d51a..76a3e073b 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -120,8 +120,6 @@ impl TestedDialects { let only_statement = statements.pop().unwrap(); if !canonical.is_empty() { - println!("Canonical: {canonical}"); - println!("Reformed: {only_statement}"); assert_eq!(canonical, only_statement.to_string()) } only_statement diff --git a/tests/sqlparser_mysql.rs b/tests/sqlparser_mysql.rs index b2b089e60..96013df1b 100644 --- a/tests/sqlparser_mysql.rs +++ b/tests/sqlparser_mysql.rs @@ -15,7 +15,6 @@ //! is also tested (on the inputs it can handle). use matches::assert_matches; -use sqlparser; use sqlparser::ast::Expr; use sqlparser::ast::Value; use sqlparser::ast::*;