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 secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp