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

feat: Generalizing CREATE SEQUENCE #3090

Merged
merged 15 commits into from
Mar 8, 2024

Conversation

VaggelisD
Copy link
Collaborator

@VaggelisD VaggelisD commented Mar 6, 2024

Fixes #3072

Generalize CREATE SEQUENCE among Oracle, T-SQL, Postgres, DuckDB, Snowflake.

Design Notes:

  • Added UnloggedProperty that exists in Postgres for tables and sequences
  • Made parsing of CREATE SEQUENCE order agnostic as dialects had differences between them
  • Non-repeating options from Oracle's dialect were encapsulated in an OPTIONS dictionary to simplify parsing
  • Future note: It seems that some overlapping options are also parsed in exp.GeneratedAsIdentityColumnConstraint, maybe a refactor between them is favorable?
  • 2nd iteration: Added GlobalProperty(post-create properly) and SharingProperty (Oracle post-name) to accommodate for regressions on a command identity , which enabled it's full roundtrip.

Docs

Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let's try to refactor this by leveraging _parse_properties() (see comment).

sqlglot/generator.py Outdated Show resolved Hide resolved
sqlglot/tokens.py Show resolved Hide resolved
sqlglot/expressions.py Outdated Show resolved Hide resolved
sqlglot/expressions.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments @VaggelisD

sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
@mpf82
Copy link
Contributor

mpf82 commented Mar 8, 2024

Great idea, but is the idea "only" to allow parsing and generating within the same dialect?

Because, if you also want to support transpiling, by looking at the code and the tests, I am not sure if Oracle's NOCYCLE and NOCACHE are correctly supported or will be correctly transpiled.

Side note: NOSHARD is added twice in CREATE_SEQUENCE

@georgesittas
Copy link
Collaborator

Hey Mike, thanks for the review / feedback 👋

is the idea "only" to allow parsing and generating within the same dialect?

For now, that's what we're aiming for. The goal is to allow fine-grained parsing of the CREATE SEQUENCE statement, so that at minimum it can be roundtripped correctly for a given dialect, while also allowing one to analyze its AST. Some basic variants of this statement should already be transpilable as is, but aiming to cover all possible options is too much work, given the number of dialects supporting CREATE SEQUENCE, and the ROI is questionable imo.

Perhaps at some point in the future we could refactor this to control more easily which options / properties are generated per dialect to improve the transpilation.

P.S. just double-checking, but is this related to transpilation, or did you also notice an issue for the Oracle -> Oracle case?

I am not sure if Oracle's NOCYCLE and NOCACHE are correctly supported

@mpf82
Copy link
Contributor

mpf82 commented Mar 8, 2024

P.S. just double-checking, but is this related to transpilation, or did you also notice an issue for the Oracle -> Oracle case?

Aftert looking again, I guess I mean just transpiling.

I was kinda confused when looking at

        cache = expression.args.get("cache")
        if cache is None:
            cache_str = ""
        elif cache is True:
            cache_str = " CACHE"
        else:
            cache_str = f" CACHE {cache}"

and not seeing NOCACHE in the code and also not seeing a test case.
But other options, such as NOSCALE seem to validate just fine.

So NOCACHE and others must be generated from options = self.expressions(expression, key="options", flat=True, sep=" "), right?
Then I think I understand how it works.

Which also means that if you construct something like "CREATE SEQUENCE seq START WITH 1 NO CACHE NOCACHE CACHE 10" it will also also "validate", correct?
Which is fine, since sqlglot's a parser, not a validator.

@georgesittas
Copy link
Collaborator

georgesittas commented Mar 8, 2024

That's correct, these options are generated using the expressions call you mentioned. And yes - the final statement will also be accepted, which is fine.

Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few final cleanup suggestions, nice work @VaggelisD.

sqlglot/generator.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
VaggelisD and others added 7 commits March 8, 2024 16:46
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
sqlglot/expressions.py Outdated Show resolved Hide resolved
@georgesittas georgesittas merged commit a38db01 into tobymao:main Mar 8, 2024
5 checks passed
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.

None yet

3 participants