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

Update sql.bnf to add create statement overrides for use the schema create #646

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

griffio
Copy link
Contributor

@griffio griffio commented Sep 6, 2024

Add CREATE stubs that can be overridden in dialects where needed to order statements

SqlDelight TreeUtil forInitializationStatements is responsible for ordering the sql statements when schema.create is generated - https://github.com/cashapp/sqldelight/blob/d7cc00eabc133b43f38a1e75f70af7e786dd00d7/sqldelight-compiler/src/main/kotlin/app/cash/sqldelight/core/lang/util/TreeUtil.kt#L255

Note:- Migrations don't use this ordering and is preferred way of creating more complex schema

Currently CREATE TABLE statements are ordered first - some database support CREATE statements that execute before e.g CREATE SCHEMA sales needs to be run before CREATE TABLE sales.Orders

What an updated TreeUtil would look like using the overrides of create statements that may be available

  val databases= ArrayList<PsiElement>()
  val views = ArrayList<SqlCreateViewStmt>()
  val tables = ArrayList<SqlCreateTableStmt>()
  val creators = ArrayList<PsiElement>()
  val miscellanious = ArrayList<PsiElement>()

  forEach { file ->
    file.sqlStatements()
      .filter { (label, _) -> label.name == null }
      .forEach { (_, sqlStatement) ->
        when {
          sqlStatement.createSchemaStmt != null -> databases.add(sqlStatement.createSchemaStmt!!)
          sqlStatement.createExtensionStmt != null -> databases.add(sqlStatement.createExtensionStmt!!)
          sqlStatement.createTableStmt != null -> tables.add(sqlStatement.createTableStmt!!)
          sqlStatement.createViewStmt != null -> views.add(sqlStatement.createViewStmt!!)
          sqlStatement.createTriggerStmt != null -> creators.add(sqlStatement.createTriggerStmt!!)
          sqlStatement.createIndexStmt != null -> creators.add(sqlStatement.createIndexStmt!!)
          else -> miscellanious.add(sqlStatement)
        }
      }
  }

  databases.forEach { body(it.rawSqlText()) } // some schema elements must exist before tables

e.g Postgresql.bnf would override create_schema_stmt with the dialect implementation and TreeUtil would be able to use the statements

create_schema_stmt ::= CREATE 'SCHEMA' [ IF NOT EXISTS ] schema_name {
   extends = "com.alecstrong.sql.psi.core.psi.impl.SqlCreateSchemaStmtImpl"
   implements = "com.alecstrong.sql.psi.core.psi.SqlCreateSchemaStmt"
   override = true
}

@AlecKazakova
Copy link
Collaborator

Note:- Migrations don't use this ordering and is preferred way of creating more complex schema

is there a use case for having schema modifiers outside of migration files? In mobile sqlite it makes sense for fresh installs but I was assuming for every other distribution it would just run migration files (and therefore be run in the order written)

@griffio
Copy link
Contributor Author

griffio commented Sep 7, 2024

Note:- Migrations don't use this ordering and is preferred way of creating more complex schema

is there a use case for having schema modifiers outside of migration files? In mobile sqlite it makes sense for fresh installs but I was assuming for every other distribution it would just run migration files (and therefore be run in the order written)

Initially, I was going to just add grammar for PostgreSql CREATE SCHEMA sqldelight/sqldelight#3761 in Sqldelight

🕵️ Then, I was looking into a viable approach with regards to "Wrong execution order of CREATE EXTENSION" sqldelight/sqldelight#5166
e.g Where the .sq. file is used to create the whole schema without migration files

It could get a bit messy where someone has a use-case for their.sq as a fresh database init script, even thinking about when DROP SCHEMA ..., DROP TABLE ... would need to be at the top to clear out a schema...

Maybe - I will stick 🏒 with fixing sqldelight/sqldelight#3761 for now and then see if there is any calls for use outside of .sqm

Also with further testing TableNameMixin https://github.com/AlecKazakova/sql-psi/blob/b0c8897030745ee4afcf2dd448c7567960f0bf4a/core/src/main/kotlin/com/alecstrong/sql/psi/core/psi/mixins/TableNameMixin.kt#L46 doesn't check for database/schema - such as sales.Orders and accounts.Orders are considered duplicate Table already defined with Orders

@AlecKazakova
Copy link
Collaborator

yea - I think even for that original issue I would prefer a fix in SQLDelight. We have a similar issue with views where a view can be defined before a table when it needs the table to exist when CREATE VIEW is called. I'm not sure offhand where that sorting logic is but ideally we just extend that to support other schema types.

Or something more generic where we always preserve .sq file ordering unless there's a reason it would break and we need to sort.

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.

2 participants