MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Alexey Kopytov Date:October 29 2009 9:42am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3200)
Bug#47925
View as plain text  
Hi Martin,

The patch looks mostly good to me. There are only a few minor issues, 
please see the inlined comments.

I'm going to approve the patch now, hope you'll have a chance to fix the 
minor stuff before pushing.

On 27.10.09 17:05, Martin Hansson wrote:
> === modified file 'sql/item.cc'
> --- a/sql/item.cc	2009-10-13 04:43:27 +0000
> +++ b/sql/item.cc	2009-10-27 14:04:59 +0000
> @@ -6866,72 +6866,60 @@ void resolve_const_item(THD *thd, Item *
[skipped]
> +    if (field->type() == MYSQL_TYPE_DATE ||
> +        field->type() == MYSQL_TYPE_DATETIME)
>       {
[skipped]
> +      enum_mysql_timestamp_type type= MYSQL_TIMESTAMP_ERROR;
> +
> +      if (field->type() == MYSQL_TYPE_DATE)
> +        type= MYSQL_TIMESTAMP_DATE;
> +
> +      if (field->type() == MYSQL_TYPE_DATETIME)
> +        type= MYSQL_TIMESTAMP_DATETIME;
> +

4 calls to field-type(). Perhaps it makes sense to store it in a variable?

> === modified file 'sql/item_cmpfunc.cc'
> --- a/sql/item_cmpfunc.cc	2009-10-05 05:27:36 +0000
> +++ b/sql/item_cmpfunc.cc	2009-10-27 14:04:59 +0000
> @@ -636,56 +636,51 @@ int Arg_comparator::set_compare_func(Ite
>     return 0;
>   }
>
> -
>   /**
> -  @brief Convert date provided in a string to the int representation.
> -
> -  @param[in]   thd        thread handle
> -  @param[in]   str        a string to convert
> -  @param[in]   warn_type  type of the timestamp for issuing the warning
> -  @param[in]   warn_name  field name for issuing the warning
> -  @param[out]  error_arg  could not extract a DATE or DATETIME
> +  Parse date provided in a string to a MYSQL_TIME.
>
> -  @details Convert date provided in the string str to the int
> -    representation.  If the string contains wrong date or doesn't
> -    contain it at all then a warning is issued.  The warn_type and
> -    the warn_name arguments are used as the name and the type of the
> -    field when issuing the warning.  If any input was discarded
> -    (trailing or non-timestampy characters), was_cut will be non-zero.
> -    was_type will return the type str_to_datetime() could correctly
> -    extract.
> -
> -  @return
> -    converted value. 0 on error and on zero-dates -- check 'failure'
> +  @param[in]   thd        Thread handle
> +  @param[in]   str        A string to convert
> +  @param[in]   warn_type  Type of the timestamp for issuing the warning
> +  @param[in]   warn_name  Field name for issuing the warning
> +  @param[out]  error_arg  Could not parse a DATE or DATETIME value
> +

Wrong comment, error_arg is not an argument of get_mysql_time_from_str().

> +  Parses a date provided in the string str into a MYSQL_TIME object. If the
> +  string contains an incorrect date or doesn't correspond to a date at all
> +  then a warning is issued. The warn_type and the warn_name arguments are used
> +  as the name and the type of the field when issuing the warning. If any input
> +  was discarded (trailing or non-timestamp-y characters), return value will be
> +  TRUE.
> +
> +  @return Result in l_time.
> +  @retval FALSE Success.
> +  @retval True Indicates failure.

"@return" and "@retval"s correspond to different values.

>   */
>
> -static ulonglong
> -get_date_from_str(THD *thd, String *str, timestamp_type warn_type,
> -                  char *warn_name, bool *error_arg)
> +bool get_mysql_time_from_str(THD *thd, String *str, timestamp_type warn_type,
> +                             const char *warn_name, MYSQL_TIME *l_time)
>   {
> -  ulonglong value= 0;
> +  bool value;
>     int error;
> -  MYSQL_TIME l_time;
> -  enum_mysql_timestamp_type ret;
> +  enum_mysql_timestamp_type timestamp_type;
>
> -  ret= str_to_datetime(str->ptr(), str->length(),&l_time,
> -                       (TIME_FUZZY_DATE | MODE_INVALID_DATES |
> -                        (thd->variables.sql_mode&
> -                         (MODE_NO_ZERO_IN_DATE | MODE_NO_ZERO_DATE))),
> -&error);
> +  timestamp_type=
> +    str_to_datetime(str->ptr(), str->length(), l_time,
> +                    (TIME_FUZZY_DATE | MODE_INVALID_DATES |
> +                     (thd->variables.sql_mode&
> +                      (MODE_NO_ZERO_IN_DATE | MODE_NO_ZERO_DATE))),
> +&error);
>
> -  if (ret == MYSQL_TIMESTAMP_DATETIME || ret == MYSQL_TIMESTAMP_DATE)
> -  {
> +  if (timestamp_type == MYSQL_TIMESTAMP_DATETIME ||
> +      timestamp_type == MYSQL_TIMESTAMP_DATE)
>       /*
>         Do not return yet, we may still want to throw a "trailing garbage"
>         warning.
>       */
> -    *error_arg= FALSE;
> -    value= TIME_to_ulonglong_datetime(&l_time);
> -  }
> +    value= FALSE;
>     else
>     {
> -    *error_arg= TRUE;
> +    value= TRUE;
>       error= 1;                                   /* force warning */
>     }
>
> @@ -698,6 +693,38 @@ get_date_from_str(THD *thd, String *str,
>   }
>
>
> +/**
> +  @brief Convert date provided in a string to the int representation.
> +
> +  @param[in]   thd        thread handle
> +  @param[in]   str        a string to convert
> +  @param[in]   warn_type  type of the timestamp for issuing the warning
> +  @param[in]   warn_name  field name for issuing the warning
> +  @param[out]  error_arg  could not extract a DATE or DATETIME
> +
> +  @details Convert date provided in the string str to the int
> +    representation.  If the string contains wrong date or doesn't
> +    contain it at all then a warning is issued.  The warn_type and
> +    the warn_name arguments are used as the name and the type of the
> +    field when issuing the warning.  If any input was discarded
> +    (trailing or non-timestampy characters), was_cut will be non-zero.

There is no "was_cut" argument.

>
> === modified file 'sql/time.cc'
> --- a/sql/time.cc	2009-06-17 14:56:44 +0000
> +++ b/sql/time.cc	2009-10-27 14:04:59 +0000
> @@ -965,20 +965,22 @@ calc_time_diff(MYSQL_TIME *l_time1, MYSQ
>       0   - a == b
>       1   - a>  b
>
> -  NOTES
> -    TIME.second_part is not considered during comparison
>   */
>
> -int
> -my_time_compare(MYSQL_TIME *a, MYSQL_TIME *b)
> +int my_time_compare(MYSQL_TIME *a, MYSQL_TIME *b)
>   {
>     my_ulonglong a_t= TIME_to_ulonglong_datetime(a);
>     my_ulonglong b_t= TIME_to_ulonglong_datetime(b);
>

I noticed that this is the only place in the server code where 
my_ulonglong is used.

I don't know if casting ulonglong (returned by 
TIME_to_ulonglong_datetime()) to my_ulonglong is safe on all platforms, 
but their definitions in my_global.h differ, so I think it would be good 
to replace my_ulonglong to ulonglong since we are now actually making 
use of this function.

Best regards,
Alexey.
Thread
bzr commit into mysql-5.1-bugteam branch (martin.hansson:3200)Bug#47925Martin Hansson27 Oct
  • Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3200)Bug#47925Alexey Kopytov29 Oct