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@mariadb.org