From d8b8b5affab7d3842eec3edbae051a9a3898b0bb Mon Sep 17 00:00:00 2001 From: Sergei Petrunia Date: Thu, 24 Mar 2016 02:09:17 +0300 Subject: [PATCH] Fix a PS re-execution problem and code cleanup - Make Item_XXX::cleanup() clean List orderby_fields. (Items survive across PS re-executions. Cached_item don't, because they keep pointers to fix_field'ed items, etc) - Move List out into Group_bound_tracker. --- sql/item_windowfunc.cc | 26 +++++----------- sql/item_windowfunc.h | 70 ++++++++++++++++++++++++++++++++++++++++-- sql/sql_window.cc | 40 ------------------------ 3 files changed, 74 insertions(+), 62 deletions(-) diff --git a/sql/item_windowfunc.cc b/sql/item_windowfunc.cc index e204ddd8a29..1add0fa68db 100644 --- a/sql/item_windowfunc.cc +++ b/sql/item_windowfunc.cc @@ -178,29 +178,21 @@ void Item_window_func::setup_partition_border_check(THD *thd) void Item_sum_rank::setup_window_func(THD *thd, Window_spec *window_spec) { /* TODO: move this into Item_window_func? */ - for (ORDER *curr= window_spec->order_list->first; curr; curr=curr->next) - { - Cached_item *tmp= new_Cached_item(thd, curr->item[0], TRUE); - orderby_fields.push_back(tmp); - } + peer_tracker.init(thd, window_spec->order_list); clear(); } void Item_sum_dense_rank::setup_window_func(THD *thd, Window_spec *window_spec) { /* TODO: consider moving this && Item_sum_rank's implementation */ - for (ORDER *curr= window_spec->order_list->first; curr; curr=curr->next) - { - Cached_item *tmp= new_Cached_item(thd, curr->item[0], TRUE); - orderby_fields.push_back(tmp); - } + peer_tracker.init(thd, window_spec->order_list); clear(); } bool Item_sum_dense_rank::add() { - if (test_if_group_changed(orderby_fields) > -1) - dense_rank++; + if (peer_tracker.check_if_next_group()) + dense_rank++; return false; } @@ -209,7 +201,7 @@ bool Item_sum_dense_rank::add() bool Item_sum_rank::add() { row_number++; - if (test_if_group_changed(orderby_fields) > -1) + if (peer_tracker.check_if_next_group()) { /* Row value changed */ cur_rank= row_number; @@ -237,7 +229,7 @@ void Item_window_func::advance_window() bool Item_sum_percent_rank::add() { row_number++; - if (test_if_group_changed(orderby_fields) > -1) + if (peer_tracker.check_if_next_group()) { /* Row value changed. */ cur_rank= row_number; @@ -248,11 +240,7 @@ bool Item_sum_percent_rank::add() void Item_sum_percent_rank::setup_window_func(THD *thd, Window_spec *window_spec) { /* TODO: move this into Item_window_func? */ - for (ORDER *curr= window_spec->order_list->first; curr; curr=curr->next) - { - Cached_item *tmp= new_Cached_item(thd, curr->item[0], TRUE); - orderby_fields.push_back(tmp); - } + peer_tracker.init(thd, window_spec->order_list); clear(); } diff --git a/sql/item_windowfunc.h b/sql/item_windowfunc.h index ae5a153707b..a0cba79fee1 100644 --- a/sql/item_windowfunc.h +++ b/sql/item_windowfunc.h @@ -6,6 +6,54 @@ class Window_spec; + +int test_if_group_changed(List &list); + +/* A wrapper around test_if_group_changed */ +class Group_bound_tracker +{ + List group_fields; +public: + void init(THD *thd, SQL_I_List *list) + { + for (ORDER *curr = list->first; curr; curr=curr->next) + { + Cached_item *tmp= new_Cached_item(thd, curr->item[0], TRUE); + group_fields.push_back(tmp); + } + } + + void cleanup() + { + group_fields.empty(); + } + + /* + Check if the current row is in a different group than the previous row + this function was called for. + The new row's group becomes the current row's group. + */ + bool check_if_next_group() + { + if (test_if_group_changed(group_fields) > -1) + return true; + return false; + } + + int compare_with_cache() + { + List_iterator li(group_fields); + Cached_item *ptr; + int res; + while ((ptr= li++)) + { + if ((res= ptr->cmp_read_only())) + return res; + } + return 0; + } +}; + /* ROW_NUMBER() OVER (...) @@ -74,7 +122,7 @@ protected: longlong row_number; // just ROW_NUMBER() longlong cur_rank; // current value - List orderby_fields; + Group_bound_tracker peer_tracker; public: void clear() { @@ -111,6 +159,11 @@ public: } void setup_window_func(THD *thd, Window_spec *window_spec); + void cleanup() + { + peer_tracker.cleanup(); + Item_sum_int::cleanup(); + } }; @@ -136,7 +189,7 @@ public: class Item_sum_dense_rank: public Item_sum_int { longlong dense_rank; - List orderby_fields; + Group_bound_tracker peer_tracker; /* XXX(cvicentiu) This class could potentially be implemented in the rank class, with a switch for the DENSE case. @@ -167,6 +220,11 @@ class Item_sum_dense_rank: public Item_sum_int void setup_window_func(THD *thd, Window_spec *window_spec); + void cleanup() + { + peer_tracker.cleanup(); + Item_sum_int::cleanup(); + } }; /* TODO-cvicentiu @@ -365,7 +423,13 @@ class Item_sum_percent_rank: public Item_sum_window_with_context, longlong cur_rank; // Current rank of the current row. longlong row_number; // Value if this were ROW_NUMBER() function. - List orderby_fields; + Group_bound_tracker peer_tracker; + + void cleanup() + { + peer_tracker.cleanup(); + Item_sum_window_with_context::cleanup(); + } /* Helper function so that we don't cast the context every time. */ Window_context_row_count* get_context_() diff --git a/sql/sql_window.cc b/sql/sql_window.cc index c189342fc91..8462b000801 100644 --- a/sql/sql_window.cc +++ b/sql/sql_window.cc @@ -387,46 +387,6 @@ public: ///////////////////////////////////////////////////////////////////////////// -/* A wrapper around test_if_group_changed */ -class Group_bound_tracker -{ - List group_fields; -public: - void init(THD *thd, SQL_I_List *list) - { - for (ORDER *curr = list->first; curr; curr=curr->next) - { - Cached_item *tmp= new_Cached_item(thd, curr->item[0], TRUE); - group_fields.push_back(tmp); - } - } - - /* - Check if the current row is in a different group than the previous row - this function was called for. - The new row's group becomes the current row's group. - */ - bool check_if_next_group() - { - if (test_if_group_changed(group_fields) > -1) - return true; - return false; - } - - int compare_with_cache() - { - List_iterator li(group_fields); - Cached_item *ptr; - int res; - while ((ptr= li++)) - { - if ((res= ptr->cmp_read_only())) - return res; - } - return 0; - } -}; - /* Window frame bound cursor. Abstract interface.