Skip to content

Add support for generated virtual columns with expression #1051

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 22, 2023
Merged

Add support for generated virtual columns with expression #1051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9618cda
Add support for generated virtual columns with expression
takluyver Nov 19, 2023
05ac5bc
Extend generated column tests for SQLite & add for MySQL
takluyver Nov 20, 2023
ec986b0
Remove outdated comment
takluyver Nov 20, 2023
f0d0f5b
Remove redundant check in tests
takluyver Nov 21, 2023
6d69a43
Preserve presence/absence of VIRTUAL for generated columns
takluyver Nov 22, 2023
2887501
Remove some leftover debugging
takluyver Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 31 additions & 24 deletions src/ast/ddl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,7 @@ pub enum ColumnOption {
generated_as: GeneratedAs,
sequence_options: Option<Vec<SequenceOptions>>,
generation_expr: Option<Expr>,
generation_expr_mode: Option<GeneratedExpressionMode>,
},
}

Expand Down Expand Up @@ -639,25 +640,25 @@ impl fmt::Display for ColumnOption {
generated_as,
sequence_options,
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, " )")?;
}
}
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(())
}
GeneratedAs::ByDefault => {
write!(f, "GENERATED BY DEFAULT AS IDENTITY")?;
} 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() {
Expand All @@ -672,17 +673,13 @@ impl fmt::Display for ColumnOption {
}
Ok(())
}
GeneratedAs::ExpStored => {
let expr = generation_expr.as_ref().unwrap();
write!(f, "GENERATED ALWAYS AS ({expr}) STORED")
}
},
}
}
}
}

/// `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))]
Expand All @@ -692,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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks good to me -- I don't have a better name for this

Virtual,
Stored,
}

fn display_constraint_name(name: &'_ Option<Ident>) -> impl fmt::Display + '_ {
struct ConstraintName<'a>(&'a Option<Ident>);
impl<'a> fmt::Display for ConstraintName<'a> {
Expand Down
4 changes: 2 additions & 2 deletions src/ast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
20 changes: 18 additions & 2 deletions src/parser/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -4247,16 +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 _ = self.parse_keywords(&[Keyword::STORED]);
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 {
Ok((GeneratedAs::Always, None))
}?;

Ok(Some(ColumnOption::Generated {
generated_as: GeneratedAs::ExpStored,
generated_as: gen_as,
sequence_options: None,
generation_expr: Some(expr),
generation_expr_mode: expr_mode,
}))
} else {
Ok(None)
Expand Down
12 changes: 12 additions & 0 deletions tests/sqlparser_mysql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,18 @@ 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)";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding additional support to the AST so you can recover the original SQL --- as implemented now this change will not permit people to know when the original query had VIRTUAL or not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Virtual is the default, so adding the marker doesn't make a logical difference (at least in the databases I found that support this). Elsewhere in the tests, it looks like you don't preserve differences like this - e.g. SHOW COLUMNS vs SHOW FIELDS in the MySQL tests. But if you'd like to preserve this one, I'm happy to do that. 🙂

If so, I would probably introduce an enum called something like GenExpressionQualifier (though that's an ugly name...), which could be None, Virtual or Stored. The Stored option would then be redundant with GeneratedAs::ExpStored.

(For my next PR, I'm planning to make the GENERATED ALWAYS keywords optional - as they are in MySQL and SQLite. Let me know if you want the presence or absence of those to be preserved too.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elsewhere in the tests, it looks like you don't preserve differences like this - e.g. SHOW COLUMNS vs SHOW FIELDS in the MySQL tests. But if you'd like to preserve this one, I'm happy to do that. 🙂

Yes, it is true that this crate is not always consistent. However, for newly added code let's try and preserve the ability to round trip the AST (though I see there is no note describing that detail on https://github.com/sqlparser-rs/sqlparser-rs -- I will try and add a note when I have time)

Let me know if you want the presence or absence of those to be preserved too.)

yes, please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, the latest commit (6d69a43) preserves the distinction between VIRTUAL, STORED and no qualifier. As I mentioned, that does mean there are two fields which both record if STORED was used. That's not so tidy, but it preserves compatibility for any code that's checking for GeneratedAs::ExpStored.

I've called the new field & enum generation_expr_mode & GeneratedExpressionMode, which are both quite a mouthful. Let me know if you have a better name for this. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filed #1052 to document this property

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);
}

#[test]
fn parse_quote_identifiers() {
let sql = "CREATE TABLE `PRIMARY` (`BEGIN` INT PRIMARY KEY)";
Expand Down
13 changes: 12 additions & 1 deletion tests/sqlparser_sqlite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,18 @@ 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().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);
}

#[test]
fn test_placeholder() {
// In postgres, this would be the absolute value operator '@' applied to the column 'xxx'
Expand Down Expand Up @@ -435,7 +447,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice drive by cleanup

dialects: vec![Box::new(SQLiteDialect {}), Box::new(GenericDialect {})],
options: None,
}
Expand Down