maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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