Hello Sergei, Please find a new version of the patch that addresses your review suggestions. Also, see comments inline. Thanks. On 05/28/2014 09:17 PM, Sergei Golubchik wrote: <skip>
=== modified file 'mysql-test/r/ps_2myisam.result' --- mysql-test/r/ps_2myisam.result 2013-11-20 11:05:39 +0000 +++ mysql-test/r/ps_2myisam.result 2014-04-15 04:25:48 +0000 @@ -3256,7 +3256,7 @@ values ( 50, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10 ) ; Warnings: Warning 1265 Data truncated for column 'c15' at row 1 -Warning 1265 Data truncated for column 'c16' at row 1 +Note 1265 Data truncated for column 'c16' at row 1
What bug number is this for? Doesn't look like any of those five that you've listed.
It converts 1.0e+10 to datetime "2001-00-00 00:00:00". It looks correct to produce a note instead of a warning here.
Would you like it to be reported separately?
At least, committed separately, if possible. I found "bzr shelve" command to be very useful in cases like that.
I reported and pushed it as a separate bug: MDEV-6287 Bad warning level when inserting a DATETIME value into a TIME column
=== modified file 'sql/field.h' --- sql/field.h 2014-03-26 21:25:38 +0000 +++ sql/field.h 2014-04-11 10:32:12 +0000 @@ -94,6 +94,28 @@ inline uint get_set_pack_length(int elem
/** + Tests if field type is temporal and has date part, + i.e. represents DATE, DATETIME or TIMESTAMP types in SQL. + + @param type Field type, as returned by field->type(). + @retval true If field type is temporal type with date part. + @retval false If field type is not temporal type with date part. +*/ +inline bool is_temporal_type_with_date(enum_field_types type) +{ + switch (type) + { + case MYSQL_TYPE_DATE: + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP:
What about MYSQL_TYPE_DATETIME2, MYSQL_TYPE_TIMESTAMP2 types?
They should never appear in this context.
MYSQL_TYPE_XXX2 can only be returned from field->real_type(), and should never be returned from item->field_type().
Could you add them with an assert then, please? Like
case MYSQL_TYPE_DATE2: case MYSQL_TYPE_DATETIME2: case MYSQL_TYPE_TIMESTAMP2: DBUG_ASSERT(0); // impossible case MYSQL_TYPE_DATE: case MYSQL_TYPE_DATETIME: case MYSQL_TYPE_TIMESTAMP: return true;
Done.
=== modified file 'sql/item_cmpfunc.cc' --- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -881,7 +886,8 @@ void Arg_comparator::set_datetime_cmp_fu
longlong get_datetime_value(THD *thd, Item ***item_arg, Item **cache_arg, - Item *warn_item, bool *is_null) + Item *warn_item, bool *is_null, + bool convert_time_to_datetime)
do you need this convert_time_to_datetime? It might be enough to use is_temporal_type_with_date(warn_item->field_type())
Conversion is needed only if args[0] is datetime and arg[1] is TIME, or the other way around.
When both args[0] and args[1] are TIMEs, no conversion is needed for performance purposes. So knowing only warn_item->field_type() is not enough.
This chunk implements this logic:
--- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -593,6 +593,11 @@ int Arg_comparator::set_compare_func(Ite switch (type) { case TIME_RESULT: cmp_collation.collation= &my_charset_numeric; + if ((a[0]->field_type() == MYSQL_TYPE_TIME && + is_temporal_type_with_date(b[0]->field_type())) || + (b[0]->field_type() == MYSQL_TYPE_TIME && + is_temporal_type_with_date(a[0]->field_type()))) + convert_time_to_datetime= true;
Yes, that's what I mean. Instead of passing convert_time_to_datetime from the caller, you can put this logic inside get_datetime_value():
if ((item->field_type() == MYSQL_TYPE_TIME && is_temporal_type_with_date(warn_item->field_type())) ...
this if() means that TIME value will be compared as datetime, and thus needs to be converted.
Done. Thant worked fine. Thanks.
Regards, Sergei