← Back to team overview

maria-developers team mailing list archive

  1. maria-developers team
  2. Mailing list archive
  3. Message #05682

Re: Review request: mdev-4634

  Thread Previous • Date Previous • • Thread Next 
Hi Sergei,

Thanks for review!


On 06/11/2013 09:33 PM, Sergei Golubchik wrote:

Hi, Alexander!

This looks like a lot of changes for what you explained the reason of
the bug was. There will be lots of "whys" below...


The fix itself was quite small. However, an attempt
to write a reasonable test case revealed a few other bugs.
I just tried to fixing everything in a single patch.

Okey. Let's do everything separately.
These are separate reports for the problems
found during writing a test case for 4634:

https://mariadb.atlassian.net/browse/MDEV-4652
https://mariadb.atlassian.net/browse/MDEV-4653
https://mariadb.atlassian.net/browse/MDEV-4654

Here's a new one, found today:
https://mariadb.atlassian.net/browse/MDEV-4651




On Jun 11, Alexander Barkov wrote:

=== modified file 'sql/item.cc'
--- sql/item.cc	2013-03-17 06:41:22 +0000
+++ sql/item.cc	2013-06-11 11:52:43 +0000
@@ -252,7 +252,8 @@ String *Item::val_string_from_decimal(St
  String *Item::val_string_from_date(String *str)
  {
    MYSQL_TIME ltime;
-  if (get_date(&ltime, TIME_FUZZY_DATE) ||
+  uint time_flag= field_type() == MYSQL_TYPE_TIME ? TIME_TIME_ONLY : 0;
+  if (get_date(&ltime, TIME_FUZZY_DATE | time_flag) ||


Why?


        str->alloc(MAX_DATE_STRING_REP_LENGTH))
    {
      null_value= 1;

=== modified file 'sql/item_func.cc'
--- sql/item_func.cc	2013-04-06 13:14:46 +0000
+++ sql/item_func.cc	2013-06-11 10:54:50 +0000
@@ -2456,7 +2456,8 @@ bool Item_func_min_max::get_date(MYSQL_T
    {
      Item **arg= args + i;
      bool is_null;
-    longlong res= get_datetime_value(thd, &arg, 0, compare_as_dates, &is_null);
+    longlong res= get_datetime_value(thd, &arg, 0, compare_as_dates,
+                                     &is_null, fuzzy_date);


why is that? to handle CONVERT_TZ(GREATEST(...)) ?
That is to pass NO_ZERO_DATE down?

I'm not sure it's a good idea. Strictly speaking, only the greatest
value should be NO_ZERO_DATE, not every single one.

I'd suggest to add check_date in Item_func_min_max::get_date() instead.


      /* Check if we need to stop (because of error or KILL) and stop the loop */
      if (thd->is_error() || args[i]->null_value)
@@ -2469,6 +2470,11 @@ bool Item_func_min_max::get_date(MYSQL_T
        min_max= res;
    }
    unpack_time(min_max, ltime);
+
+  if (!(fuzzy_date & TIME_TIME_ONLY) &&
+      ((null_value= check_date_with_warn(ltime, fuzzy_date))))
+    return true;


Yes, that's what I mean. If you check the return value anyway, there's
no need to check it inside get_datetime_value().


    if (compare_as_dates->field_type() == MYSQL_TYPE_DATE)
    {
      ltime->time_type= MYSQL_TIMESTAMP_DATE;

=== modified file 'sql/item_timefunc.cc'
--- sql/item_timefunc.cc	2013-03-20 15:13:00 +0000
+++ sql/item_timefunc.cc	2013-06-11 10:57:09 +0000
@@ -1923,8 +1921,10 @@ void Item_date_add_interval::fix_length_
  bool Item_date_add_interval::get_date(MYSQL_TIME *ltime, uint fuzzy_date)
  {
    INTERVAL interval;
-
-  if (args[0]->get_date(ltime, TIME_NO_ZERO_DATE | TIME_FUZZY_DATE | TIME_NO_ZERO_IN_DATE) ||
+  if (args[0]->get_date(ltime,
+                        TIME_NO_ZERO_DATE | TIME_FUZZY_DATE | TIME_NO_ZERO_IN_DATE |
+                        ((args[0]->field_type() == MYSQL_TYPE_TIME) ?
+                         TIME_TIME_ONLY : 0)) ||


why is that?


        get_interval_value(args[1], int_type, &value, &interval))
      return (null_value=1);

@@ -2290,6 +2290,12 @@ bool Item_time_typecast::get_date(MYSQL_
      return 1;
    if (decimals < TIME_SECOND_PART_DIGITS)
      ltime->second_part= sec_part_truncate(ltime->second_part, decimals);
+  if (!(fuzzy_date & TIME_TIME_ONLY))
+  {
+    if (ltime->time_type == MYSQL_TIMESTAMP_TIME)
+      ltime->time_type= MYSQL_TIMESTAMP_DATETIME;


why?


+    return ((null_value= check_date_with_warn(ltime, fuzzy_date)));
+  }
    /*
      MYSQL_TIMESTAMP_TIME value can have non-zero day part,
      which we should not lose.
=== modified file 'sql/item_timefunc.h'
--- sql/item_timefunc.h	2012-12-28 12:41:46 +0000
+++ sql/item_timefunc.h	2013-06-11 11:19:03 +0000
@@ -492,6 +492,8 @@ class Item_timefunc :public Item_tempora
    Item_timefunc(Item *a,Item *b) :Item_temporal_func(a,b) {}
    Item_timefunc(Item *a, Item *b, Item *c) :Item_temporal_func(a, b ,c) {}
    enum_field_types field_type() const { return MYSQL_TYPE_TIME; }
+  int save_in_field(Field *field, bool no_conversions)
+  { return save_time_in_field(field); }


why?


  };

=== modified file 'sql/time.cc'
--- sql/time.cc	2013-01-08 20:23:03 +0000
+++ sql/time.cc	2013-06-11 10:51:31 +0000
@@ -213,6 +213,20 @@ ulong convert_month_to_period(ulong mont
  }


+bool
+check_date_with_warn(const MYSQL_TIME *ltime, uint fuzzy_date)
+{
+  int dummy_warnings;
+  if (check_date(ltime, fuzzy_date, &dummy_warnings))
+  {
+    Lazy_string_time str(ltime);
+    make_truncated_value_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
+                                 &str, MYSQL_TIMESTAMP_ERROR, 0);


please, use this function also in Item_date_typecast::get_date() and
may be in other places. you might need to pass MYSQL_TIMESTAMP_xxx
value as an argument, though.


+    return true;
+  }
+  return false;
+}
+
  /*
    Convert a timestamp string to a MYSQL_TIME value and produce a warning
    if string was truncated during conversion.



Regards,
Sergei
  Thread Previous • Date Previous • • Thread Next