maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

Looks ok to push, thanks!
One minor comment below.

On Jun 02, Alexander Barkov wrote:
> === modified file 'sql/field.h'
> --- sql/field.h	2014-03-26 21:25:38 +0000
> +++ sql/field.h	2014-06-02 10:13:23 +0000
> @@ -94,6 +94,31 @@ 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:
> +    return true;
> +  case MYSQL_TYPE_DATETIME2:
> +  case MYSQL_TYPE_TIMESTAMP2:
> +    DBUG_ASSERT(0); // field->real_type() should not get to here.

In my earlier comment, where I suggested this assert, I've put it before
other cases. That is, in optimized builds, when asserts are disabled,
is_temporal_type_with_date() would still return true for
MYSQL_TYPE_DATETIME2 and MYSQL_TYPE_TIMESTAMP2.

You've put it too late, it would return false for these types.

Admittedly, it's not terribly important, so fix it or not, as you like.

> +  default:
> +    return false;
> +  }
> +}

Regards,
Sergei