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

CI: Clippy, MySQL & Postgres #21

Merged
merged 33 commits into from
Nov 8, 2021
Merged

CI: Clippy, MySQL & Postgres #21

merged 33 commits into from
Nov 8, 2021

Conversation

billy1624
Copy link
Member

No description provided.

@billy1624 billy1624 self-assigned this Oct 22, 2021
@billy1624 billy1624 changed the title Improve CI CI: Clippy, MySQL & Postgres Oct 22, 2021
@billy1624
Copy link
Member Author

Discovery behaviour difference on MySQL vs MariaDB. Creating the same table but schema discovery results are different. @tyt2y3

MySQL, https://github.com/SeaQL/sea-schema/runs/3974821329?check_suite_focus=true#step:10:1164

Diff < left / right > :
<"CREATE TABLE `bakery` ( `id` int(8) NOT NULL AUTO_INCREMENT, `name` varchar(255), `profit_margin` double, PRIMARY KEY (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_general_ci"
>"CREATE TABLE `bakery` ( `id` int NOT NULL AUTO_INCREMENT, `name` varchar(255), `profit_margin` double, PRIMARY KEY (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_general_ci"

MariaDB, https://github.com/SeaQL/sea-schema/runs/3974821107?check_suite_focus=true#step:10:1298

Diff < left / right > :
<"CREATE TABLE `bakery` ( `id` int(8) NOT NULL AUTO_INCREMENT, `name` varchar(255), `profit_margin` double, PRIMARY KEY (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_general_ci"
>"CREATE TABLE `bakery` ( `id` int(8) NOT NULL AUTO_INCREMENT, `name` varchar(255) DEFAULT NULL, `profit_margin` double DEFAULT NULL, PRIMARY KEY (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3 COLLATE=utf8mb3_general_ci"

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 22, 2021

Both are semantically equivalent. It's hard to be verbatim exact.

@billy1624
Copy link
Member Author

Perhaps we can skip the mysql/live checks for now

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 22, 2021

Well we actually need a looser way to compare two statements, like ignoring the integer widths and 'default null'.

But Postgres is all fine?

@billy1624
Copy link
Member Author

Well we actually need a looser way to compare two statements, like ignoring the integer widths and 'default null'.

But Postgres is all fine?

All fine

@billy1624 billy1624 mentioned this pull request Nov 3, 2021
@billy1624
Copy link
Member Author

Hey @tyt2y3, seems that for MySQL 8 the information schema changed? It fail to parse the column length of integer column.

Diff < left / right > :
<"CREATE TABLE `bakery` ( `id` int(255) NOT NULL AUTO_INCREMENT, `name` varchar(255), `profit_margin` double, PRIMARY KEY (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci"
>"CREATE TABLE `bakery` ( `id` int NOT NULL AUTO_INCREMENT, `name` varchar(255), `profit_margin` double, PRIMARY KEY (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci"

GitHub Actions

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 4, 2021

I think it was deprecated in 8.
I am thinking to override the default impl of ColumnDef such that the width of integer columns are ignored.
Then it can handle the previous failed case in test suite too.

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 5, 2021

Okay a better idea is to have a normalise() method to strip out the irrelevant stuff so they will be equivalent.

@billy1624
Copy link
Member Author

Okay a better idea is to have a normalise() method to strip out the irrelevant stuff so they will be equivalent.

Strip the asserting String directly?

@billy1624 billy1624 marked this pull request as ready for review November 5, 2021 08:57
@billy1624
Copy link
Member Author

Ready for review & merge @tyt2y3

@tyt2y3 tyt2y3 merged commit 10b11a9 into master Nov 8, 2021
@tyt2y3 tyt2y3 deleted the improve-ci branch November 8, 2021 05:29
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