Hi, Jeremy! First - assume that for all your comments that I didn't explicitly reply to, there's an implicit reply "ok, will do". That is, for pretty much all of them :) On Nov 11, Jeremy Cole wrote:
- It would probably be worth visually comparing to MySQL 5.6's get_innobase_type_from_mysql_type to ensure there is nothing new there aside from my comments.
No need to. I didn't revert, but copied get_innobase_type_from_mysql_type from 5.6 and changed it in two places to account for our temporal type changes (see below). It's identical otherwise.
- It is almost certainly worth creating test cases with data files from older MariaDB and newer MySQL versions to exercise the usage of the code paths that only occur during upgrade; that doesn't need to happen here necessarily.
Yes, Elena is doing that. But I don't think mtr is suited for this kind of tests.
=== renamed file 'mysql-test/suite/innodb/t/1byte_data_int.test' =>
+ t1_SMALLINT SMALLINT, + t1_SMALLINT_UNSIGNED SMALLINT UNSIGNED, + t1_TEXT TEXT, + t1_TIME TIME, + t1_TIMESTAMP TIMESTAMP, + t1_TIMESTAMP_5 TIMESTAMP(5), + t1_TIME_4 TIME(4),
Why 4 and not 3 or 6 or some other value?
No reason. Just arbitrarily TIMESTAMP(4), DATETIME(6), TIME(4).
- (prtype & 512) = 512 AS is_unsigned + name, CASE mtype WHEN 1 THEN "DATA_VARCHAR"
+ case MYSQL_TYPE_NEWDATE: + return(DATA_INT); + case MYSQL_TYPE_TIMESTAMP: + *unsigned_flag = 0; + /* fall through */
That's the first change to get_innobase_type_from_mysql_type() as copied from 5.6. Because in 5.6 TIMESTAMP is signed, in MariaDB - unsigned. I've noticed it, because I've generated the result file on 5.6.
+ case MYSQL_TYPE_TIME: + case MYSQL_TYPE_DATETIME: + if (field->key_type() == HA_KEYTYPE_BINARY) + return(DATA_FIXBINARY); + else + return(DATA_INT);
You also need to support MYSQL_TYPE_{TIMESTAMP,TIME,DATETIME}2 for MySQL 5.6 here.
I do. That's the second difference from get_innobase_type_from_mysql_type(). In 5.6 they do a switch on field->real_type(). But in MariaDB, temporal types with microsecond have the same real_type as without, one need to look at the key_type(). Incidentally, this also covers {TIMESTAMP,TIME,DATETIME}2 from 5.6, they also have key_type() == HA_KEYTYPE_BINARY. So, this if() covers everything.
+ case MYSQL_TYPE_FLOAT:
Regards, Sergei