-
Notifications
You must be signed in to change notification settings - Fork 583
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.
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
Add support for generated columns skipping 'GENERATED ALWAYS' keywords #1058
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.