Fix anomalies in migration of integer columns in MySQL 8 (#1358)#1360
Fix anomalies in migration of integer columns in MySQL 8 (#1358)#1360paul-rouse merged 5 commits intoyesodweb:masterfrom
Conversation
parsonsmatt
left a comment
parsonsmatt
left a comment
There was a problem hiding this comment.
A few questions but otherwise looks great, thank you!
| -- The non-default display width "(1)" is kept for now to avoid differences | ||
| -- between persistent versions running against older MySQL versions (eg 5.7): | ||
| showSqlType SqlBool _ _ = "TINYINT(1)" | ||
| showSqlType SqlDay _ _ = "DATE" | ||
| showSqlType SqlDayTime _ _ = "DATETIME" | ||
| showSqlType SqlInt32 _ _ = "INT(11)" | ||
| showSqlType SqlInt32 _ _ = "INT" |
There was a problem hiding this comment.
This comment doesn't appear to agree with the code change - can you clarify?
There was a problem hiding this comment.
It's not a great comment - I'll add a commit to try to improve it! The point is I'm trying to avoid changing anything about what is put into the information schema in MySQL 5.7 and early members of the 8.X series. In 5.7, INT gets a default display width (11) anyway, so I felt it would be clearer in the future to omit it here (as was already done for BIGINT). However (1) is not the default for TINYINT, and omitting it would make MySQL 5.7 add (4) instead in the information schema. It doesn't affect the data storage for the column itself, of course, but the different information schema might cause spurious migrations in persistent, or be incompatible with some external program the user might depend upon.
| parseColumnType "tinyint" ci | ||
| | ciColumnType ci == "tinyint" || ciColumnType ci == "tinyint(1)" = return (SqlBool, Nothing) | ||
| parseColumnType "int" ci | ||
| | ciColumnType ci == "int" || ciColumnType ci == "int(11)" = return (SqlInt32, Nothing) | ||
| parseColumnType "bigint" ci | ||
| | ciColumnType ci == "bigint" || ciColumnType ci == "bigint(20)" = return (SqlInt64, Nothing) |
There was a problem hiding this comment.
does it maybe make sense to do a Just _ <- Text.stripPrefix "tinyint" pattern? That would match tinyint, tinyint(1), and tinyint(2345) etc.
There was a problem hiding this comment.
Again, I am trying to avoid changing anything for MySQL 5.7 and early 8.X. These versions will always report the display width, so I thought it best to match it exactly, as before, to avoid including cases which were not included in persistent-mysql-2.13.1.0. The addition is the matching of the bare type, eg int, which will only be reported by later 8.X versions
|
@parsonsmatt I will add a commit to improve the comment in the code, but would you like me to merge and release then? I tend to leave things |
|
I've got this one - just remembered it's sitting around. |
2 participants
Fixes #1358. I believe this remains compatible with older versions of MySQL, so should be a minor version bump.
Before submitting your PR, check that you've:
stylish-haskellon any changed files..editorconfigfile for details)After submitting your PR:
(unreleased)on the Changelog