some ddl callback clauses are duplicated

[ruslandoga]

These two ranges seem to be duplicated. The second one seems "more complete", it has exceptions on constraints and uses prefix in table renaming. Except, what are table prefixes in SQLite?

https://github.com/elixir-sqlite/ecto_sqlite3/blob/6dcc0b2410ac2e2727d6c81ecb2e96c410eb8be4/lib/ecto/adapters/sqlite3/connection.ex#L460-L562

and

https://github.com/elixir-sqlite/ecto_sqlite3/blob/6dcc0b2410ac2e2727d6c81ecb2e96c410eb8be4/lib/ecto/adapters/sqlite3/connection.ex#L564-L685

[warmwaffles]

Oh that's weird. I wonder why it wasn't caught by linting or the compiler?

[ruslandoga]

I wondered that too, so I had to double check that the clauses are in fact the same. Maybe @impl true has something to do with it? Or maybe the expressions in the clauses are "too complex".

Should we tag someone from the Elixir core team?

[warmwaffles]

It very well could be all the @impl trues. I think we should remove them to be set on the first defined method of the implementation.

[warmwaffles]

@ruslandoga table prefixes IIRC is a way to prefix table names via a config. https://hexdocs.pm/ecto_sql/Ecto.Migration.html#module-prefixes

We definitely need to support the table prefixing.

[ruslandoga]

But does sqlite support them?

sqlite> create table prefix.table(a integer);
Parse error: near "table": syntax error
  create table prefix.table(a integer);
                      ^--- error here

My understanding is, sqlite is one file, one schema, one database.

[warmwaffles]

It does not, but we can support it via <prefix>_<tablename>

[warmwaffles]

Or we could just throw our hands up in the air and raise an exception saying that is is not supported.

[ruslandoga]

I'd probably raise an exception. <prefix>_<tablename> does not really provide any data separation that is expected from separate schemas (postgresql) / databases (mysql).

[warmwaffles]

I'm okay with removing the prefix support and failing loudly. We'd need to make clear call out in the changelog that it is a breaking change and will need to be accounted for.

[ruslandoga]

I don't think anyone was using table prefixes

https://github.com/elixir-sqlite/ecto_sqlite3/blob/6dcc0b2410ac2e2727d6c81ecb2e96c410eb8be4/lib/ecto/adapters/sqlite3/connection.ex#L1802

since that would've caused the sqlite parser to throw

sqlite> create table prefix.table(a integer);
Parse error: near "table": syntax error
  create table prefix.table(a integer);
                      ^--- error here

so it's going to be a safe change. I hope.

[ruslandoga]

If it's ok with you, I'd like to try and work on all this (duplicate ddl clauses, impl true, table prefix) tomorrow :)

[warmwaffles]

I'm all good with that.

I took a stab at adding dialyzer here too to see if it would catch the duplicate function definition. But that was a mess to deal with.