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(postgres): Support generated columns #4472

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

VaggelisD
Copy link
Collaborator

Fixes #4463

Postgres supports the following column constraints for CREATE TABLE DDLs:

where column_constraint is:

[ CONSTRAINT constraint_name ]
{ 
  ...
  GENERATED ALWAYS AS ( generation_expr ) STORED |
  GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( sequence_options ) ] |
  ...
}

The former defines a generated column while the latter an identity one.

This PR overrides parser.py::_parse_generated_as_identity() which will transform the produced exp.GeneratedAsIdentityColumnConstraint into an exp.ComputedColumnConstraint(), as is already happening on spark.py.

Warning

Something that stands out as bad naming/design is the fact that exp.GeneratedAsIdentityColumnConstraint should concern only IDENTITY columns, but overtime it has grown to parse different variants of the GENERATED syntax even if semantically they're not equivalent.

This leads to the "hacky" solution of having to transform it to exp.ComputedColumnConstraint after it has been parsed, as is happening in the PR. Should we make an effort to deduplicate the two?

Docs

PSQL CREATE TABLE | PSQL GENERATED columns | PSQL IDENTITY columns

@VaggelisD VaggelisD changed the title feat(postgres): Support STORED computed columns feat(postgres): Support generated columns Dec 4, 2024
sqlglot/dialects/postgres.py Outdated Show resolved Hide resolved
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
@georgesittas georgesittas merged commit a9dca8d into main Dec 4, 2024
6 checks passed
@georgesittas georgesittas deleted the vaggelisd/psql_computed_col branch December 4, 2024 10:39
@AKST
Copy link

AKST commented Dec 4, 2024

Thanks @VaggelisD, if following a similar approach is fine, I don't think it would take much for me to add this to MySQL/SQLite to add support of the virtual keyword, happy to add that 🙂

However I can see this is not a general approach but an approach tailored to Postgres. I am correct to say the choice to go with this less general approach is a reflection of an established users issue for Postgres (my original issue #4463) & the absence of one for SQLite/MySQL? Had that not been the case and there was more available bandwidth, a more general approach would have been favoured to also handle MySQL & SQLite?

I ask, because if that's the case it may not make sense for me to do that if a more general approach is preferred for handling MySQL & SQLite as well.

@VaggelisD
Copy link
Collaborator Author

Hey @AKST, sorry for not getting back to you earlier, missed the ping here.

As I mention on the warning note, it seems like we've currently overloaded exp.GeneratedAsIdentityColumnConstraint and the handling of computed/generated columns has become a hacky afterthought.

Solving this generally would require decoupling the 2 syntax variants (GENERATED AS (expr) and GENERATED AS IDENTITY) which are similar for dialects like Postgres, but that's not trivial at this point.

The easy solution for extending this to other dialects would be to follow my approach here and tailoring exp.ComputedColumn to them; If we get around to refactoring these 2 expressions, we'll at least have the majority of the code & test cases ready.

@AKST
Copy link

AKST commented Dec 6, 2024

That's fair, I'm not sure I would be up to the task of refactoring/decoupling those 2 expressions. Is it worth following your approach here for SQLite & MySQL or just waiting till that refactor happens?

Adithyak-0926 pushed a commit to e6data/sqlglot that referenced this pull request Dec 11, 2024
* feat(postgres): Support STORED computed columns

* Update sqlglot/dialects/postgres.py

Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>

---------

Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
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.

Postgres: Unable to parse STORE Generated Column on create table
3 participants