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

MariaDB default values for LONGTEXT types don't work in Dolt #7033

Closed
xEinfachJoshua opened this issue Nov 21, 2023 · 9 comments
Closed

MariaDB default values for LONGTEXT types don't work in Dolt #7033

xEinfachJoshua opened this issue Nov 21, 2023 · 9 comments
Assignees

Comments

@xEinfachJoshua
Copy link

This create table statement works in MariaDB and not in Dolt:
error:
failed to run import job: failed to import sql: error on line 1313 for query
ENGINE=InnoDB Default ChARSET=utf8mb4 COLLATE=utf8mb4_general_ci: TEXT, BLOB, GEOMETRY, and JSON types may only have expression default
image

@timsehn
Copy link
Contributor

timsehn commented Nov 21, 2023

The query is:

CREATE TABLE `hex_hud` (
  `id` int(11) NOT NULL, 
  `identifier` varchar(100) NOT NULL DEFAULT '0',
  `settings` longtext NOT NULL DEFAULT '[]',
  `positions` longtext DEFAULT '[]'
);

@timsehn
Copy link
Contributor

timsehn commented Nov 21, 2023

Repro:

PS C:\Users\timse\dolthub\dolt> mkdir test_longtext_default


    Directory: C:\Users\timse\dolthub\dolt


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----        11/21/2023   1:06 PM                test_longtext_default


PS C:\Users\timse\dolthub\dolt> cd .\test_longtext_default\
PS C:\Users\timse\dolthub\dolt\test_longtext_default> dolt init --fun
Successfully initialized dolt data repository.
PS C:\Users\timse\dolthub\dolt\test_longtext_default> dolt sql -q "CREATE TABLE `hex_hud` (`id` int(11) NOT NULL, `identifier` varchar(100) NOT NULL DEFAULT '0', `settings` longtext NOT NULL DEFAULT '[]', `positions` longtext DEFAULT '[]');"
error on line 1 for query CREATE TABLE hex_hud (id int(11) NOT NULL, identifier varchar(100) NOT NULL DEFAULT '0', settings longtext NOT NULL DEFAULT '[]', positions longtext DEFAULT '[]'): TEXT, BLOB, GEOMETRY, and JSON types may only have expression default values

@fulghum fulghum self-assigned this Nov 21, 2023
@fulghum
Copy link
Contributor

fulghum commented Nov 21, 2023

Looks like we are matching MySQL's behavior on this one:

mysql> CREATE TABLE `hex_hud` (
    ->   `id` int(11) NOT NULL, 
    ->   `identifier` varchar(100) NOT NULL DEFAULT '0',
    ->   `settings` longtext NOT NULL DEFAULT '[]',
    ->   `positions` longtext DEFAULT '[]'
    -> );
ERROR 1101 (42000): BLOB, TEXT, GEOMETRY or JSON column 'settings' can't have a default value

Dolt does give a slightly better error message than MySQL – we include a hint that the default value must be an expression, instead of a literal. Here's the same query running successfully against MySQL with parens around the LONGTEXT default clause to make it an expression instead of a literal. The reported warning is for the integer display width, not the default expressions:

mysql> CREATE TABLE `hex_hud` (
    ->   `id` int(11) NOT NULL, 
    ->   `identifier` varchar(100) NOT NULL DEFAULT '0',
    ->   `settings` longtext NOT NULL DEFAULT ('[]'),
    ->   `positions` longtext DEFAULT ('[]')
    -> );
Query OK, 0 rows affected, 1 warning (0.01 sec)

I also confirmed that this modified query with the parens around the default value works with Dolt.

@timsehn
Copy link
Contributor

timsehn commented Nov 21, 2023

Does MariaDB allow the expression without parentheses?

@fulghum
Copy link
Contributor

fulghum commented Nov 21, 2023

Does MariaDB allow the expression without parentheses?

Yup, MariaDB does seem to support automatically converting the literal default values to an expression. (I used OneCompiler's online db test tool to confirm.)

We discussed this a little more on Discord and agreed that although this diverges from MySQL's behavior, it's a good change for us to make and helps us with compatibility with MariaDB.

I'm starting to dig in now and will get a PR opened as soon as possible.

@fulghum
Copy link
Contributor

fulghum commented Nov 21, 2023

I've made this change in the go-mysql-server module and will get that dependency updated in Dolt next. After the test suite runs, I'm happy to cut a release so you can try xampp out with Dolt again.

Thanks for taking the time to engage us and let us know about this issue that you hit! If you run into anything else while testing Dolt with xampp, please let us know! We'll be happy to dig in and help out.

I'll reply again once the release is out.

@xEinfachJoshua
Copy link
Author

xEinfachJoshua commented Nov 21, 2023 via email

@timsehn
Copy link
Contributor

timsehn commented Nov 21, 2023

Let's cut a release and get it onto the DoltHub workers.

@fulghum
Copy link
Contributor

fulghum commented Nov 22, 2023

This change was just released in Dolt version 1.28.0. We're getting the DoltHub workers upgraded to this new version so that it's ready for you to try next time you're online, too.

Don't hesitate to let us know if you hit any more snags! We're happy to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants