Skip to content
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

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

takluyver
Copy link
Contributor

SQLite, Mysql and DuckDB all let you skip the words GENERATED ALWAYS, and use AS 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 the GENERATED keyword is present, and false if the SQL skips directly to AS. I'm not sure of the naming, but it's the best I've thought of so far. Currently, if generation_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.

@coveralls
Copy link

coveralls commented Nov 29, 2023

Pull Request Test Coverage Report for Build 7052140096

  • 26 of 32 (81.25%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 87.695%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser/mod.rs 16 22 72.73%
Files with Coverage Reduction New Missed Lines %
src/ast/ddl.rs 1 83.78%
Totals Coverage Status
Change from base Build 7009122777: -0.01%
Covered Lines: 18017
Relevant Lines: 20545

💛 - 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,
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@tobyhede
Copy link
Contributor

tobyhede commented Dec 6, 2023

LGTM

Copy link
Contributor

@alamb alamb left a 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

@alamb alamb merged commit da2296e into apache:main Dec 19, 2023
@takluyver takluyver deleted the gencol-direct-as branch December 21, 2023 09:04
@takluyver
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support more forms of generated columns for SQLite dialect
4 participants