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 <b...@mariadb.com
> <mailto: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
> <mailto: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
