Add support for generated virtual columns with expression by takluyver · Pull Request #1051 · apache/datafusion-sqlparser-rs

takluyver · 2023-11-19T14:15:51Z

This allows the SQLite & MySQL syntax for generated virtual columns, addressing the second part of #1050.

-- These two statements are equivalent:
CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) VIRTUAL);
CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2));

I've done a partial survey of what different databases support for generated columns:

SQLite: generate columns from an expression, either VIRTUAL (evaluated on read) or STORED (on write), but must ALWAYS be generated - docs, syntax reference
MySQL: same options as SQLite - docs
DuckDB: only VIRTUAL supported, as of v0.9.2 - docs.
Postgres: columns generated from an expression can only be STORED & ALWAYS (as of v16, the current version), but there are additional options to generate a column from a sequence (AS IDENTITY) - docs, syntax reference
MS-SQL has GENERATED AS syntax, but it doesn't seem to overlap with the other DBs above at all - it's something to do with versioning records, with no space for arbitrary expressions - docs. I haven't tried to cover this in this PR.

It looks like the generated-column code before this PR is based on Postgres.

I've expanded the meaning of GeneratedAs::ExpStored, which the docs previously described as Postgres specific, to any column generated from an expressions and stored. GeneratedAs::Always with generation_expr present now represents a VIRTUAL column in SQLite or MySQL. It might be neater to make stored/virtual separate from always/by-default, but that would be an API break.

tobyhede · 2023-11-19T23:18:05Z

LGTM.
Is there a test for the STORED option?
And it might be worth adding tests for the MySQL dialect. Makes intention explicit and easier to track regressions.

takluyver · 2023-11-20T10:07:05Z

There's one test covering the same GENERATED AS expr STORED syntax in Postgres:

https://github.com/sqlparser-rs/sqlparser-rs/blob/c887c4e54558db3d3940c23300a384b1752cdeaf/tests/sqlparser_postgres.rs#L208-L218

But I can add this for SQLite & MySQL too. 👍

If SQLite & MySQL have the same syntax for this, do you prefer me to copy & paste the test into their respective modules, or set up one test to run with both of them? I'll copy-paste for now.

coveralls · 2023-11-20T20:01:05Z

Pull Request Test Coverage Report for Build 6961152498

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

38 of 39 (97.44%) changed or added relevant lines in 4 files are covered.
1257 unchanged lines in 8 files lost coverage.
Overall coverage increased (+0.04%) to 87.753%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/ast/ddl.rs	11	12	91.67%

Files with Coverage Reduction	New Missed Lines	%
src/dialect/mod.rs	1	90.41%
src/dialect/mssql.rs	2	86.67%
src/dialect/redshift.rs	2	78.57%
tests/sqlparser_mssql.rs	3	97.39%
tests/sqlparser_postgres.rs	26	98.06%
tests/sqlparser_common.rs	123	96.97%
src/ast/mod.rs	345	79.04%
src/parser/mod.rs	755	83.2%

Totals
Change from base Build 6914235970:	0.04%
Covered Lines:	17992
Relevant Lines:	20503

💛 - Coveralls

alamb

Thank you for this contribution @takluyver (and the review @tobyhede ). I left some comments -- let me know what you think

alamb · 2023-11-20T20:03:22Z


 fn sqlite_and_generic() -> TestedDialects {
    TestedDialects {
-        // we don't have a separate SQLite dialect, so test only the generic dialect for now


👍 nice drive by cleanup

alamb · 2023-11-20T20:04:53Z

+#[test]
+fn parse_create_table_gencol() {
+    let sql_default = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2))";
+    let sql_virt = "CREATE TABLE t1 (a INT, b INT GENERATED ALWAYS AS (a * 2) VIRTUAL)";


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

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

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

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

filed #1052 to document this property

alamb

Thank you @takluyver -- looks good to me

alamb · 2023-11-22T18:04:52Z

+#[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 {


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

takluyver · 2023-11-22T18:08:25Z

Ooops, I left some debugging bits in the tests. I've removed them now - do you prefer me to squash that into the previous commit and force-push the branch?

alamb · 2023-11-22T18:12:55Z

Ooops, I left some debugging bits in the tests. I've removed them now - do you prefer me to squash that into the previous commit and force-push the branch?

On merge all the commits will be squashed into a single commit, so I would prefer that you just push new commits to your branch / PR as it makes reviewing the deltas easier

alamb · 2023-11-22T18:16:23Z

Thanks again @takluyver

Add support for generated virtual columns with expression

9618cda

takluyver added 2 commits November 20, 2023 10:07

Extend generated column tests for SQLite & add for MySQL

05ac5bc

Remove outdated comment

ec986b0

alamb reviewed Nov 20, 2023

View reviewed changes

alamb mentioned this pull request Nov 20, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 20, 2023 apache/datafusion#8274

Closed

7 tasks

takluyver added 2 commits November 21, 2023 08:42

Remove redundant check in tests

f0d0f5b

Preserve presence/absence of VIRTUAL for generated columns

6d69a43

alamb approved these changes Nov 22, 2023

View reviewed changes

Remove some leftover debugging

2887501

alamb merged commit 640b939 into apache:main Nov 22, 2023

takluyver deleted the gencol-virtual branch November 22, 2023 18:17

This was referenced Nov 22, 2023

Support more forms of generated columns for SQLite dialect #1050

Closed

Add support for generated columns skipping 'GENERATED ALWAYS' keywords #1058

Merged

Conversation

takluyver commented Nov 19, 2023

Uh oh!

tobyhede commented Nov 19, 2023

Uh oh!

takluyver commented Nov 20, 2023

Uh oh!

coveralls commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 6961152498

Warning: This coverage report may be inaccurate.

💛 - Coveralls

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

alamb Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

takluyver Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

alamb Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

takluyver Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

alamb Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

takluyver commented Nov 22, 2023

Uh oh!

alamb commented Nov 22, 2023

Uh oh!

alamb commented Nov 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Nov 20, 2023 •

edited

Loading