Hi Alexander! I was looking through this patch as I am rather familiar with this code. I didn't take time to test this out, but maybe you can explain if this is a possible concern or not:
index 4a95189..7d1532c 100644 --- a/sql/item_sum.cc +++ b/sql/item_sum.cc @@ -404,7 +404,7 @@ bool Item_sum::register_sum_func(THD *thd, Item **ref) for (sl= thd->lex->current_select; sl && sl != aggr_sel && sl->master_unit()->item; sl= sl->master_unit()->outer_select() ) - sl->master_unit()->item->with_sum_func= 1; + sl->master_unit()->item->get_with_sum_func_cache()->set_with_sum_func(); } thd->lex->current_select->mark_as_dependent(thd, aggr_sel, NULL); @@ -484,7 +484,6 @@ void Item_sum::mark_as_sum_func() cur_select->n_sum_items++; cur_select->with_sum_func= 1; const_item_cache= false; - with_sum_func= 1; with_field= 0; window_func_sum_expr_flag= false; } diff --git a/sql/item_sum.h b/sql/item_sum.h index 96f1153..37f3fe0 100644 --- a/sql/item_sum.h +++ b/sql/item_sum.h @@ -582,6 +582,8 @@ class Item_sum :public Item_func_or_sum void mark_as_window_func_sum_expr() { window_func_sum_expr_flag= true; } bool is_window_func_sum_expr() { return window_func_sum_expr_flag; } virtual void setup_caches(THD *thd) {}; + + bool with_sum_func() const { return true; } }; For Item_sum::register_sum_func, if sl->master_unit()->item is an Item_sum_sum for example, an Item_sum won't have get_with_sum_func_cache() overwritten so it will be the base Item::get_with_sum_func_cache(), which returns null and you will crash. Am I missing something? Is it impossible for sl->master_unit()->item to be an Item_sum_... subclass? I am not a very big fan of the get_with_sum_func_cache() indirection required and would prefer, if possible to call set_with_sum_func() directly. Perhaps behind the scenes the set function can do that and throw an assert if the call is illegal? (Just an opinion, not something I have a very very strong opinion on. Also, I have a feeling that it's sufficient to keep just join_with_sum_func. I can't really think of a place where that was not the intent anyway, but those few cases where copy_with_sum_func is used need to be analysed throughly to make sure. Regards, Vicențiu PS: Yes, the review assigned to me is coming :) On Tue, 29 May 2018 at 15:07 Alexander Barkov <b...@mariadb.com> wrote: > Hello Sanja, > > I recently worked on MDEV-16309 and had hard time to find > which Item classes in the hierarchy: > - have always with_sum_func==true > - have always with_sum_func==false > - have variable with_sum_func > > To make it sure, before actually working on MDEV-16309, > I had to create a separate working tree and did with > Item::with_sum_func the same change that we previously > did for Item::with_subselect in: > > MDEV-14517 Cleanup for Item::with_subselect and Item::has_subquery() > (which you reviewed) > > - I find the code easier to read this way > (instead of searching for all possible assignments, > one can search who overrides the method) > - Also, some Item can become smaller in size. > > It's pity to throw the patch away. So perhaps we could just apply it. > > Can you please have a look? > > Thanks. > > > I the meanwhile I'll create a new MDEV for it > (with a similar description to MDEV-14517) > _______________________________________________ > 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 >
_______________________________________________ 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