maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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 <bar@xxxxxxxxxxx
> <mailto:bar@xxxxxxxxxxx>> 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@xxxxxxxxxxxxxxxxxxx
>     <mailto:maria-developers@xxxxxxxxxxxxxxxxxxx>
>     Unsubscribe : https://launchpad.net/~maria-developers
>     More help   : https://help.launchpad.net/ListHelp
>