Skip to content

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
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