maria-developers team mailing list archive

[Vicențiu Ciorbaru <cvicentiu@xxxxxxxxx>]

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