Re: [Maria-developers] With_sum_func_cache

Hi Vicențiu, Thanks for looking into this: On 05/31/2018 10:16 PM, Vicențiu Ciorbaru wrote:

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 don't think it is possible. It's Item_subselect. class st_select_lex_unit: public st_select_lex_node { ... /* not NULL if unit used in subselect, point to subselect item */ Item_subselect *item; ... };

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.

I found three places where get_with_sum_func_cache() is used: 1. In case of udf_handler::fix_fields() we have to call func->get_with_sum_func_cache() and check it for NULL, to distinguish between: - a regular udf function which has a cash, and - an aggregate udf, which does not have a cache (as it's always "with sum func"). 2. In case of count_field_types() we also have to catch Item_std_field, which does not have a cache. So the call for func->get_with_sum_func_cache() is needed, and we need to check the result for NULL again. 3. JOIN::rollup_init(). This is the only case were your approach would work. I don't think we need a wrapper to honor one out of three cases.

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.

My task was to do an equivalent code change that does not change the execution flow. I can add comments near all calls for copy_with_sum_func(), and one can study separately if they are really necessary.

Regards, Vicențiu

PS: Yes, the review assigned to me is coming :)

Great. Thank you!

On Tue, 29 May 2018 at 15:07 Alexander Barkov mailto:bar@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 mailto:maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp