-
Notifications
You must be signed in to change notification settings - Fork 605
Add support for generated columns skipping 'GENERATED ALWAYS' keywords #1058
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
Pull Request Test Coverage Report for Build 7052140096
💛 - Coveralls |
src/ast/ddl.rs
Outdated
@@ -600,6 +600,8 @@ pub enum ColumnOption { | |||
sequence_options: Option<Vec<SequenceOptions>>, | |||
generation_expr: Option<Expr>, | |||
generation_expr_mode: Option<GeneratedExpressionMode>, | |||
/// false if 'GENERATED ALWAYS' is skipped (option starts with AS) | |||
generated_kw: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference, but I would be inclined to expand the name to generated_keyword
for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, I've done that renaming.
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me -- thank you @takluyver and thank you @tobyhede for the review
Thanks! |
SQLite, Mysql and DuckDB all let you skip the words
GENERATED ALWAYS
, and useAS
to introduce a column generated from an expression. See the links in #1051 for details.I've introduced an extra field in
ColumnOption::Generated
-generated_kw
is true if theGENERATED
keyword is present, and false if the SQL skips directly toAS
. I'm not sure of the naming, but it's the best I've thought of so far. Currently, ifgeneration_expr
is None,generated_kw
can only be true (and is assumed to be when recreating the SQL).Perhaps a neater design would be to have different
ColumnOption
kinds for generated-from-expression vs. generated-from-sequence vs. whatever else. But that would require an API break again.Closes #1050.