maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergei,

The solutions looks fine

Two comments below, about the implementation.

ok to push after addressing them

On May 03, Sergei Petrunia wrote:
> revision-id: 85cc56875e9 (mariadb-10.2.43-75-g85cc56875e9)
> parent(s): 3b6c04f44c4
> author: Sergei Petrunia
> committer: Sergei Petrunia
> timestamp: 2022-05-03 14:06:27 +0300
> message:
> 
> MDEV-19398: Assertion `item1->type() == Item::FIELD_ITEM ...
> 
> Window Functions code tries to minimize the number of times it
> needs to sort the select's resultset by finding "compatible"
> OVER (PARTITION BY ... ORDER BY ...) clauses.
> 
> This employs compare_order_elements(). That function assumed that
> the order expressions are Item_field-derived objects (that refer
> to a temp.table). But this is not always the case: one can
> construct queries order expressions are arbitrary item expressions.
> 
> Add handling for such expressions: sort them according to the window
> specification they appeared in.
> This means we cannot detect that two compatible PARTITION BY clauses
> that use expressions can share the sorting step.
> But at least we won't crash.
> can share the same sort
> 
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index 989ca0c8803..457849a7569 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -8624,6 +8624,7 @@ bool st_select_lex::add_window_def(THD *thd,
>      fields_in_window_functions+= win_part_list_ptr->elements +
>                                   win_order_list_ptr->elements;
>    }
> +  win_def->win_spec_number= window_specs.elements;
>    return (win_def == NULL || window_specs.push_back(win_def));
>  }
>  
> @@ -8651,6 +8652,7 @@ bool st_select_lex::add_window_spec(THD *thd,
>                                   win_order_list_ptr->elements;
>    }
>    thd->lex->win_spec= win_spec;
> +  win_spec->win_spec_number= window_specs.elements;
>    return (win_spec == NULL || window_specs.push_back(win_spec));
>  }
>  
> diff --git a/sql/sql_window.cc b/sql/sql_window.cc
> index 3ef751bc5b9..fbf7aca0197 100644
> --- a/sql/sql_window.cc
> +++ b/sql/sql_window.cc
> @@ -312,15 +312,44 @@ setup_windows(THD *thd, Ref_ptr_array ref_pointer_array, TABLE_LIST *tables,
>  #define CMP_GT         2    // Greater then
>  
>  static
> -int compare_order_elements(ORDER *ord1, ORDER *ord2)
> +int compare_order_elements(ORDER *ord1, int weight1,
> +                           ORDER *ord2, int weight2)
>  {
>    if (*ord1->item == *ord2->item && ord1->direction == ord2->direction)
>      return CMP_EQ;
>    Item *item1= (*ord1->item)->real_item();
>    Item *item2= (*ord2->item)->real_item();
> -  DBUG_ASSERT(item1->type() == Item::FIELD_ITEM &&
> -              item2->type() == Item::FIELD_ITEM); 
> -  ptrdiff_t cmp= ((Item_field *) item1)->field - ((Item_field *) item2)->field;
> +
> +  bool item1_field= (item1->type() == Item::FIELD_ITEM);
> +  bool item2_field= (item2->type() == Item::FIELD_ITEM);
> +
> +  ptrdiff_t cmp;
> +  if (item1_field && item2_field)
> +    cmp= ((Item_field *) item1)->field - ((Item_field *) item2)->field;

1. why is this order stable? I'd rather sort by item->field->field_index
2. please, add an assert, that item1->field->table == item2->field->table

> +  else if (item1_field && !item2_field)
> +    return CMP_LT;
> +  else if (!item1_field && item2_field)
> +    return CMP_LT;
> +  else
> +  {
> +    /*
> +      Ok, item1_field==NULL and item2_field==NULL.
> +      We're not able to compare Item expressions. Order them according to
> +      their passed "weight" (which comes from Window_spec::win_spec_number):
> +    */
> +    if (weight1 != weight2)
> +      cmp= weight1 - weight2;
> +    else
> +    {
> +      /*
> +        The weight is the same. That is, the elements come from the same
> +        window specification... This shouldn't happen.
> +      */
> +      DBUG_ASSERT(0);
> +      cmp= item1 - item2;
> +    }
> +  }
> +
>    if (cmp == 0)
>    {

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx