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 create_type_stmt and alter_type_stmt #535

Closed
wants to merge 1 commit into from

Conversation

griffio
Copy link
Contributor

@griffio griffio commented Jul 18, 2023

Explicitly add fake extension points for dialects to implement

This is added to sql-psi core as needs to be referenced in sql-delight to use in Database schema generation

e.g create_type_stmt psi elements must be ordered before create_table_stmt elements

closes #533

explicity add fake extension points for dialects to implement

This is added to sql-psi core as needs to be referenced in sql-delight to use in Database schema generation
@AlecKazakova
Copy link
Collaborator

i think you can accomplish this without fake rules by overriding the stmt type to include your new rules in a downstream dialect

@griffio
Copy link
Contributor Author

griffio commented Jul 18, 2023

i think you can accomplish this without fake rules by overriding the stmt type to include your new rules in a downstream dialect

I had tried it before without the Fake assignment and the PSI elements are not generated in core (That was why I assumed the fake declaration was needed for this purpose).
i.e. these are not generated without FAKE

com/alecstrong/sql/psi/core/psi/SqlAlterTypeStmt.java
com/alecstrong/sql/psi/core/psi/SqlCreateTypeStmt.java

The reason for adding into core is these need to be referenced in Database Generation - rather than having Postgres types.
I also didn't want to have to redeclare all the stmt elements in a dialect just to add a couple more.

@hfhbd
Copy link
Collaborator

hfhbd commented Aug 17, 2023

But you can simple use extension_stmt, how is this different?

@griffio
Copy link
Contributor Author

griffio commented Aug 17, 2023

@hfhbd

Yes - the override extension_stmt method is how I am proceeding with other tickets 👍

However, CREATE TYPE stmts must be executed before CREATE TABLE stmt that use the new types, the SqlDelight compiler outputs CREATE TABLE stmts first ( 🌳 TreeUtil?) in the generated database source.

The above change was tested with my branch sqldelight/sqldelight@master...griffio:sqldelight:add-postgres-enum-type

Where I had to add ordering for createtype in TreeUtil sqldelight/sqldelight@master...griffio:sqldelight:add-postgres-enum-type#diff-f77d27d75a872743e1e5330b54cbc046fbdebc09bb4a95c496b40ca9979b7281

Maybe this generated ordering is not an issue for migrations? - so am happy to go with extension_stmt

🕊️ I believe that the core grammar should have these (CREATE/ALTER/DROP) place-holder (abstract) statements that can be overridden by Dialects rather than using extension_stmt all the time. See #533

Thanks 🍔

@griffio griffio closed this Mar 26, 2024
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 create type statement in core grammer
3 participants