adding support for MariaDB dialect

[BananaMasterz]

@simolus3 if you could review these changes (with your help I will have more ready for review soon but I need your help for smooth implementation as per our discussion, for example still trying to figure out a smooth way to set the escape character as well as the datetimes)

[simolus3]

Maybe we should make these fields on the SqlDialect enum to avoid the chain of ternary expressions here?

Then it could just be dialect.typeForBool, for instance.

[BananaMasterz]

That's a good idea, could you give me some sample code? Not sure where I would put the checks inside of the enum.

[simolus3]

We could define SqlType like this:

enum SqlDialect {
  /// Use sqlite's sql dialect. This is the default option and the only
  /// officially supported dialect at the moment.
  sqlite(
    textType: 'TEXT',
    integerType: 'INTEGER',
    realType: 'REAL',
    blobType: 'BLOB',
  ),

  /// PostgreSQL (currently supported in an experimental state)
  postgres(
    booleanType: 'boolean',
    textType: 'text',
    integerType: 'bigint',
    blobType: 'bytea',
    realType: 'float8',
  );

  final String? booleanType;
  final String textType;
  final String integerType;
  final String realType;
  final String blobType;

  const SqlDialect({
    required this.textType,
    required this.integerType,
    required this.realType,
    required this.blobType,
    this.booleanType,
  });
}

And then do something like this in mapping.dart:

  /// Returns a suitable representation of this type in SQL.
  String sqlTypeName(GenerationContext context) {
    final dialect = context.dialect;

    // ignore: unnecessary_cast
    switch (this as DriftSqlType<Object>) {
      case DriftSqlType.bool:
        return dialect.booleanType ?? dialect.integerType;
      // other cases

[BananaMasterz]

Ah yeah, that's nice and clean. I will try it tomorrow since it's late now.

By the way, case DriftSqlType.bigInt: is empty and goes to case DriftSqlType.int:, MariaDB has both int and bigInt, which one would you recommend we use? I see in postgre bigint was used. In my last commit I set for, see if you think it's ok

[BananaMasterz]

We should be aware that should some dialect require a 3rd option for some type, like
dialect.booleanType ?? dialect == SqlDialect.mysql? 'smeTypeLiteral': 'someOtherTypeLiteral' we'll probably have to use a ternary expression for it but I think that's ok and probably won't happen since null will be rare I think

[simolus3]

By the way, case DriftSqlType.bigInt: is empty and goes to case DriftSqlType.int:, MariaDB has both int and bigInt, which one would you recommend we use?

I see how this is a bit confusing, but drift actually only supports one integer type in the database: 64-bit signed integers, which I think is a bigint in MariaDb as well. The only difference between int and bigInt in drift is that the later gets mapped to a BigInt in Dart. This can make a difference for web apps, since a Dart int on the web is just a double under a trench coat and can't represent large integers properly. But in the database, the expectation is that DriftSqlType.int and DriftSqlType.bigInt both represent the same type.

We should be aware that should some dialect require a 3rd option for some type, like
dialect.booleanType ?? dialect == SqlDialect.mysql? 'smeTypeLiteral': 'someOtherTypeLiteral'

Right, but we could just set the booleanType property for the affected dialect then, right?

[BananaMasterz]

but drift actually only supports one integer type in the database: 64-bit signed integers, which I think is a bigint in MariaDb as well

correct, ok so I'll remove the bigIntType I added and use BIGINT for mariadb got it.

Right, but we could just set the booleanType property for the affected dialect then, right?

I'm talking about a case of a 2nd different fallback.

case DriftSqlType.bool:
  return dialect.booleanType ?? dialect.integerType;

we did this, but now imagine some dialect doesn't have a boolean like sqlite, and it uses dialect.textType instead, then it would become something like this:

case DriftSqlType.bool:
  return dialect.booleanType ?? dialect == SqlDialect.someDialect ? dialect.textType : dialect.integerType;

This is obviously an extreme example and it doesn't even make sense for bools sepcifically, but could occur for some other type (although I doubt it since the dialects aren't that many, and their differences aren't that great either so I'm just thinking of an extremely unlikely scenario)

[simolus3]

Yeah, I guess we could cheat and set booleanType: 'TEXT' for that dialect. But then, maybe we just do that for sqlite3 as well. Let's make booleanType non-nullable and sett it to INT for the sqlite3 dialect.

[BananaMasterz]

That's a great idea since it will always have to be something

[BananaMasterz]

could you check 82a8f84, I feel like we should add some mariadb-specific documentation as to why this if exists and it doesn't just move on cuz somebody in the future might go like "ah this isn't doing anything let's delete it" and not think of mariadb

[simolus3]

could you check 82a8f84, I feel like we should add some mariadb-specific documentation as to why this if exists and it doesn't just move on cuz somebody in the future might go like "ah this isn't doing anything let's delete it" and not think of mariadb

Good idea, and tests are an excellent way to ensure this :) Could you add something like this in drift/test/database/expressions/in_expression_test.dart under the group('values block?

    test('avoids generating empty tuples', () {
      // Some dialects don't support the `x IS IN ()` form, so we should avoid
      // it and replace it with the direct constant (since nothing can be a
      // member of the empty set). sqlite3 seems to do the same thing, as
      // `NULL IN ()` is `0` and not `NULL`.
      expect(innerExpression.isIn([]), generates('FALSE'));
      expect(innerExpression.isNotIn([]), generates('TRUE'));
    });

[simolus3]

I think we should move a bunch of duplicated logic checking for mariadb to common classes, but otherwise this looks great!

Do you want to contribute a package like drift_mariadb in the extras/ folder as well? I think it would make it easier for others to use drift with MariaDb. With such package, we (I'm very happy to help with that) could also run integration tests for drift against MariaDb as part of the CI pipeline. This ensures that the MariaDb integration continues to work in the future.

[simolus3]

I think we should be moving these to the NoTransactionDelegate class which is created by the connection implementation. The class could have a String Function(int depth) field for savepoints, releases and rollbacks. Here, we could then call _startCommand = _parent._delegate.savepoint(depth), _commitCommand = _parent._delegate.release(depth) and so on.

[BananaMasterz]

Good idea, check my implementation on my last commit if you agree with it and its documentation.

[simolus3]

We're using this check in a lot of places. Maybe we should have a String escapeChar field on SqlDialect? SqlDialect could then have a method like this

String escape(String identifier) => '$escapeChar$identifier$escapeChar';

We could then replace usages with dialect.escape(entityName) instead of duplicating the check everywhere. In the (unlikely) event that we'll ever have to use a third escape character, that will make it easier to migrate.

[BananaMasterz]

I had thought of that too but didn't know if I should do it. Since you thought it's a good idea too, I went ahead and did it. In generation_context we have this now:
String identifier(String columnName) => dialect.escape(columnName);
which seems a bit redundant but not sure... probably ok tho, since it's easier to do something.x instead of something.something.y(z). And for postgres I put double quotes as the escape char, I assume that's ok since I didn't find any checks for otherwise.

Also in Column.escapedName I still can't do it other than hardcoded since not access to dialect at this point. Only reason I pushed it hardcoded is cuz you told me to in order for you to easily find where to change it, should you choose to do it.

[BananaMasterz]

Do you want to contribute a package like drift_mariadb in the extras/ folder as well?

I could copy paste extras\drift_postgres and replace for mariadb where needed. Would that work?

And silly question, should it be:
class MariaDBDatabase extends DelegatedDatabase {}
or
class MariaDatabase extends DelegatedDatabase {}
😅 I don't like either

[simolus3]

Yeah, a copy-paste variant of that package would be a good starting point 👍 Thanks! And MariaDBDatabase is fine. The duplicate database in the name is kind of weird right? :D But I think MariaDatabase is worse somehow.

[BananaMasterz]

Yeah, a copy-paste variant of that package would be a good starting point

Great cuz that's what I did haha (if you want to add stuff in the test, you are free to do so since I'm not the best at those)

But I think MariaDatabase is worse somehow.

Yep, agree! I went with MariaDBDatabase. Contempleted a bit on DB vs Db but I figured if their own website has it DB who am I to change it?

[simolus3]

Contempleted a bit on DBvs Db but I figured if their own website has it DB who am I to change it?

Right. And I'm not as consistent as I should be with this either, but the Dart styleguide also suggests fully capitalizing two-letter acronyms, it's probably for the best.

I have setup integration tests talking to MariaDb, but unfortunately they fail at the very first statement:

00:00 +0: test/drift_mariadb_test.dart: can use no-op transactions                                                                                                                                                                                                        
Drift: Sent CREATE TABLE IF NOT EXISTS `users` (`id` BIGINT NOT NULL PRIMARY KEY AUTOINCREMENT, `name` TEXT NOT NULL, `birth_date` BIGINT NOT NULL, `profile_picture` BLOB NULL, `preferences` TEXT NULL); with args []
00:00 +0 -1: test/drift_mariadb_test.dart: can use no-op transactions [E]                                                                                                                                                                                                 
  MySQLServerException [1064]: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ' `name` TEXT NOT NULL, `birth_date` BIGINT NOT NULL, `profile_picture` BLOB N...' at line 1

I'm not that familiar with MariaDb anymore, do you recognize what might be causing this?

[BananaMasterz]

but the Dart styleguide also suggests fully capitalizing two-letter acronyms

Ah nice, didn't know this.

AUTOINCREMENT

should be AUTO_INCREMENT

[BananaMasterz]

I'm working with migration a bit (see 0963022), and assuming legacy_alter_table is unique to SQLite. Could you check if my implementation to ignore it for MariaDB is correct/acceptable? I don't see anything for postgres here either and I'm curious if it's the same as SQLite or if it's just not supported.

Finally in alterTable there's this:

final schemaQuery = await _db.customSelect(
  'SELECT type, name, sql FROM sqlite_master WHERE tbl_name = ?;',
  variables: [Variable<String>(tableName)],
).get();

and I'm not sure what to do with it since I don't have the sqlite_master table and I see _schema isn't used for that yet.

[BananaMasterz]

In migration.dart at alterTable I keep getting this error:

Cannot delete or update a parent row: a foreign key constraint fails, which was thrown here:

at Step 6

await _db.customStatement('DROP TABLE ${context.identifier(tableName)}');

I even put await _db.customStatement('SET FOREIGN_KEY_CHECKS=0;'); right above it to make sure foreign key checks are disabled.
When I try the steps in DBeaver (copy pasting the commands that drift runs) and running them one by one, it runs normally... I think it has something to do with the note here

DROP TABLE automatically commits the current active transaction, unless you use the TEMPORARY keyword.

so it commits and drift loses the connection for that session or something? cuz in MariaDB the SET FOREIGN_KEY_CHECKS=0 is per session. Do you have any better ideas as to why this doesn't work in drift but works in DBeaver?

[simolus3]

Do you have any better ideas as to why this doesn't work in drift but works in DBeaver?

Not really, can you reproduce this by manually running the individual statements via the mariadb client package? Maybe it's because we're using a pool internally and the different statements are running on different TCP connections to MariaDB? That would be crazy though.

FWIW, I don't think we necessarily need the alterTable API in an early version. Its documentation clearly mentions that it's tailored towards a migration procedure necessary for sqlite3. In postgres and MariaDB, we have actual ALTER TABLE support, users should probably construct those manually instead of re-creating the table every time.
(If you manage to port the alterTable API that would be awesome, I'm just saying it's not necessary right away from my point of view - the postgres port certainly doesn't support it either)

[BananaMasterz]

ah you are right... I was so stuck getting this to work:

await m.alterTable(TableMigration(
  table,
  columnTransformer: {
    for (final column in dateTimeColumns)
      column: DateTimeExpressions.fromUnixEpoch(column.dartCast<int>()),
  },
));

using

static Expression<DateTime> fromUnixEpoch(Expression<int> unixEpoch) {
  return _DependingOnDateTimeExpression(
    forTimestamps: unixEpoch.dartCast(),
    forIsoString: FunctionCallExpression('FROM_UNIXTIME', [unixEpoch]),
  );
}

that I got caught up in it. Might try and figure that out with custom statements instead.

Maybe it's because we're using a pool internally and the different statements are running on different TCP connections to MariaDB? That would be crazy though.

That would be crazy indeed, I did also try single connection from the package but that didn't do it so I've no idea why it ignores SET FOREIGN_KEY_CHECKS=0. Could be something with the package itself or MariaDB specific. No idea. I don't think it has something to do with drift tho.

[simolus3]

I've merged develop into this branch, some of the integration tests for MariaDB pass now 🎉

Some of the test failures need to be handled in generated code (for instance, MariaDB doesn't seem to support index variables so we can't do something like a = ?1 OR b = ?1). I can work on those since they'll probably require complicated changes in the generator. Other failures might be coming from the implementation (like not being able to bind Uint8Lists and double variables being selected as strings in SELECT ?) - if you have an idea on what's going wrong there, it would be great if you could have a look.

[BananaMasterz]

some of the integration tests for MariaDB pass now 🎉

That's great!

so we can't do something like a = ?1 OR b = ?1

Can you give me an example of when we would do this? Wouldn't we just do a = ? OR b = ? and then do (x, x) and have the same variable? Or is it just so we don't pass duplicate variables?

not being able to bind Uint8Lists and double variables being selected as strings in SELECT ?

Would it be easy for you to give me a code sample that doesn't work to investigate?

Also, did you merge 78d6064 as well? cuz that has a hardcoded backtick in columns.dart that we don't have access to dialect... unless backtick as escape works fine in SQLite too...

[BananaMasterz]

for some reason I'm running the build runner with the latest commits and suddenly the database.g.dart file is written for sqlite and not for mariadb. I'm not sure which change cause this, do you have any idea as to which one could be? It's like the writer overrides my dialect parameter set to mariadb

Update: it seems like in drift_dev analysis/options.dart

  List<SqlDialect> get supportedDialects {
    final dialects = dialect?.dialects;
    final singleDialect = dialect?.dialect;

    if (dialects != null) {
      return dialects;
    } else if (singleDialect != null) {
      return [singleDialect];
    } else {
      return const [SqlDialect.sqlite];
    }
  }

the problem is traced here since when I replace the else clause with return const [SqlDialect.mariadb]; it generates the code correctly.

[simolus3]

It's like the writer overrides my dialect parameter set to mariadb

Could you post the build.yaml file you're using for that? I think it should hit the second branch if you have set MariaDB as a dialect.

[simolus3]

Can you give me an example of when we would do this? Wouldn't we just do a = ? OR b = ? and then do (x, x) and have the same variable?

Yes, that's the idea. Unfortunately, if the user explicitly uses the same variable multiple times, like there:

drift/extras/integration_tests/drift_testcases/lib/database/database.dart

Lines 74 to 79 in 525ab27

	'friendshipsOf': ''' SELECT
	f.really_good_friends, "user".**
	FROM friendships f
	INNER JOIN users "user" ON "user".id IN (f.first_user, f.second_user) AND
	"user".id != :user
	WHERE (f.first_user = :user OR f.second_user = :user)''',

We need to come up with something. My idea is to leave the query as-is for sqlite3 and postgres, but for MariaDB we need to use three distinct variables and bind the same value to all of them. This wouldn't be too hard if it was just variables, but we also support Dart components and array variables which can allocate a dynamic amount of variables. So the code needs to reflect that, which is not that easy to implement. The internal query model of the generator doesn't provide enough information for that at the moment, I'll take a look at adding support for that transformation.

Would it be easy for you to give me a code sample that doesn't work to investigate?

drift/extras/integration_tests/drift_testcases/lib/suite/crud_tests.dart

Lines 169 to 173 in 525ab27

	test('Uint8List', () {
	final list = Uint8List.fromList(List.generate(12, (index) => index));
	expect(evaluate(Variable<Uint8List>(list)), completion(list));
	});

Note: This might be fixed by simply applying the same transformation for MariaDB:

drift/extras/integration_tests/drift_testcases/lib/suite/crud_tests.dart

Lines 123 to 129 in 525ab27

	if (database.executor.dialect == SqlDialect.postgres) {
	// 'SELECT'ing values that don't come from a table return as String
	// by default, so we need to explicitly cast it to the expected type
	// https://www.postgresql.org/docs/current/typeconv-select.html
	effectiveExpr = expr.cast<T>();
	} else {
	effectiveExpr = expr;

[BananaMasterz]

Could you post the build.yaml file you're using for that? I think it should hit the second branch if you have set MariaDB as a dialect.

I do not have a build.yaml as I'm building with the default options. I have set the dialect on my LazyDatabase and on my DelegatedDatabase as mariadb which worked before. Maybe I broke something by copy pasting the current branch's code into my ...\AppData\Local\Pub\Cache\hosted\pub.dev\drift-2.10.0 and drift_dev-2.10.0 accordingly?

[simolus3]

I think this is a consequence of #2536 then. Since most users only need sqlite3 support and since we don't have stable support for another dialect, I think the generated code should be optimized for sqlite3 only by default.

When you add a build.yaml file with these contents, MariaDB support should work again:

targets:
  $default:
    builders:
      drift_dev:
        options:
          sql:
            dialects:
              - sqlite # you could even remove this one and get mariadb-optimized code if you don't need sqlite3
              - mariadb

Maybe I broke something by copy pasting the current branch's code into my ...\AppData\Local\Pub\Cache\hosted\pub.dev\drift-2.10.0

Are you aware of dependency overrides? You can put this into your pubspec to make it use your drift checkout:

dependency_overrides:
  drift:
    path: path/to/your/drift/clone/drift
  drift_dev:
    path: path/to/your/drift/clone/drift_dev

[BananaMasterz]

I think this is a consequence of #2536 then.

ah yeah that could be it...

I think the generated code should be optimized for sqlite3 only by default.

Absolutely agree until stable support is ensured.

When you add a build.yaml file with these contents, MariaDB support should work again:

You are correct, works like a charm now. I was caught off-guard cuz I didn't need it before and the code generation ran fine so it didn't cross my mind.

Are you aware of dependency overrides?

Well I am now 😅 thanks for the tip!

[simolus3]

I'm fine with merging this now since this doesn't interfere with the other dialects, but as you can see from the MariaDB integration tests, some things are still broken. Especially the transaction failures are a bit concerning.

So we can merge this as it is and fix it later since it's still experimental, or if you want to you can also take a look at this before. I'm fine with either approach.

[BananaMasterz]

I'm also fine with merging, I will be taking a look at those transaction errors since I haven't had an issue with normal or nested transaction, as you said it's still experimental so it should be ok, maybe we could throw in some quick documentation / TODO until the it's fixed and the tests pass?

[simolus3]

Alright, thanks a lot for your contribution! 🚀