maria-developers team mailing list archive

[Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx>]

Hi Alexander!

Comments inline. OK to push otherwise.

> diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
> index fc3f81a..6fcfec4 100644
> --- a/sql/item_cmpfunc.cc
> +++ b/sql/item_cmpfunc.cc
> @@ -118,8 +118,11 @@ static int cmp_row_type(Item* item1, Item* item2)
>      0  otherwise
>  */
>
> -bool Type_handler_hybrid_field_type::aggregate_for_comparison(Item
**items,
> -                                                              uint
nitems)
> +bool
> +Type_handler_hybrid_field_type::aggregate_for_comparison(const char
*funcname,
> +                                                         Item **items,
> +                                                         uint nitems,
> +                                              bool
treat_int_to_uint_as_decimal)
The comment above this function is outdated. Please update. Also, weird
indenting here to try and stay within 80 characters. I'd say new line for
each
parameter in the function and 4 space indent for each line.
>  {
>    uint unsigned_count= items[0]->unsigned_flag;
>    /*
> @@ -471,8 +489,16 @@ int Arg_comparator::set_cmp_func(Item_func_or_sum
*owner_arg,
>    set_null= set_null && owner_arg;
>    a= a1;
>    b= a2;
> -  m_compare_handler=
Type_handler::get_handler_by_cmp_type(item_cmp_type(*a1,
> -
*a2));
> +  Item *tmp_args[2];
> +  tmp_args[0]= *a1;
> +  tmp_args[1]= *a2;
>
This could simply be Item *tmp_args[2]= {*a1, *a2};
>
> +  Type_handler_hybrid_field_type tmp;
> +  if (tmp.aggregate_for_comparison(owner_arg->func_name(), tmp_args, 2,
false))
> +  {
> +    DBUG_ASSERT(thd->is_error());
> +    return 1;
> +  }
> +  m_compare_handler= tmp.type_handler();
>    return m_compare_handler->set_comparator_func(this);
>  }
>
> @@ -3033,7 +3063,7 @@ bool
Item_func_case::prepare_predicant_and_values(THD *thd, uint *found_types)
>    add_predicant(this, (uint) first_expr_num);
>    for (uint i= 0 ; i < ncases / 2; i++)
>    {
> -    if (add_value_skip_null(this, i * 2, &have_null))
> +    if (add_value_skip_null("CASE..WHEN", this, i * 2, &have_null))
If you look at the test results, the CASE..WHEN is the only operation that
ends up in CAPS, while the rest are lowercase. I'd say to have this in lower
case as well, for consistency.
>        return true;
>    }
>    all_values_added(&tmp, &type_cnt, &m_found_types);

Vicentiu


On Fri, 30 Dec 2016 at 22:40 Alexander Barkov <bar@xxxxxxxxxxx> wrote:

> Hello Vicențiu,
>
> can you please review a patch for MDEV-11692.
>
> The patch is for bb-10.2-ext.
>
> Thanks!
>