maria-developers team mailing list archive

[Sergey Petrunya <psergey@xxxxxxxxxxxx>]

Hi Monty,

On Fri, Jun 04, 2010 at 05:36:59PM +0300, Michael Widenius wrote:
> Note that this is not a full review, but just a quick scan of some of
> the things in the commit. (One suspicious thing found...)

Thanks for the feedback! Reply summary:
- This commit was primarily to get a buildbot run, hence the presence of loads
  of commented-out old code and lack of real comments. I'll address this a bit
  later.
- The suspicious thing confirmed and fixed.
- Style suggestions followed.


> >>>>> "Sergey" == Sergey Petrunya <psergey@xxxxxxxxxxxx> writes:
> 
> <cut>
> 
> Sergey> === modified file 'sql/item_cmpfunc.cc'
> Sergey> --- a/sql/item_cmpfunc.cc	2010-05-25 06:32:15 +0000
> Sergey> +++ b/sql/item_cmpfunc.cc	2010-06-04 13:40:57 +0000
> Sergey> @@ -5733,6 +5733,8 @@
> Sergey>        It's a field from an materialized semi-join. We can substitute it only
> Sergey>        for a field from the same semi-join.
> Sergey>      */
> Sergey> +#if 0
> Sergey> +    psergey3:remove:
> 
> Please don't ever use #if 0; Instead use something like:
> 
> #ifdef LEFT_FOR_TESTING_WILL_BE_REMOVED_BY_PSERGEY_SOON
> 
> Even better to just remove the code (after all, we can always find it
> in bzr)
> 
> <cut>
> Sergey> -      if (item->field->table->reginfo.join_tab >= first)
> Sergey> +      //if (item->field->table->reginfo.join_tab >= first)
> 
> Same here; Don't leave the old code around
> 
> <cut>
> 
> Sergey> +bool join_tab_execution_startup(JOIN_TAB *tab)
> Sergey>  {
> Sergey> +  DBUG_ENTER("join_tab_execution_startup");
> Sergey>    Item_in_subselect *in_subs;
> 
> Please put DBUG_ENTER after all declarations.
> (So that we have same layout in C and C++)
> 
> <cut>
> 
> Sergey> +#if 0 
> 
> Replace with proper #if or remove code
> <cut>
> 
> Sergey> +++ b/sql/sql_select.cc	2010-06-04 13:40:57 +0000
> Sergey> @@ -1008,15 +1006,26 @@
> Sergey>    /*
> Sergey>      Permorm the the optimization on fields evaluation mentioned above
> Sergey>      for all on expressions.
> Sergey> -  */ 
> Sergey> -  for (JOIN_TAB *tab= join_tab + const_tables; tab < join_tab + tables ; tab++)
> Sergey> +  */
> Sergey> +
> Sergey>    {
> Sergey> -    if (*tab->on_expr_ref)
> Sergey> +    List_iterator<JOIN_TAB_RANGE> it(join_tab_ranges);
> Sergey> +    JOIN_TAB_RANGE *jt_range;
> Sergey> +    bool first= TRUE;
> 
> Wouldn't it be better to set first to const_tables and then to 0 ?
> 
> Sergey> +    while ((jt_range= it++))
> Sergey>      {
> Sergey> +      for (JOIN_TAB *tab= jt_range->start + (first ? const_tables : 0); 
> 
> If you do the above, then you can just do 'jt_range->start + first' here
> 
> Sergey> +           tab < jt_range->end; tab++)
> Sergey> +      {
> Sergey> +        if (*tab->on_expr_ref)
> Sergey> +        {
> Sergey> +          *tab->on_expr_ref= substitute_for_best_equal_field(*tab->on_expr_ref,
> Sergey> +                                                             tab->cond_equal,
> Sergey> +                                                             map2table);
> Sergey> +          (*tab->on_expr_ref)->update_used_tables();
> Sergey> +        }
> Sergey> +      }
> Sergey> +      first= FALSE;
> Sergey>      }
> Sergey>    }
>  
> A comment for the above outer loop would be nice.
> (It's not obvious why only the first element in join_tab_ranges has
> const tables)
> 
> Sergey> @@ -1026,6 +1035,7 @@
> Sergey>    {
> Sergey>      conds=new Item_int((longlong) 0,1);	// Always false
> Sergey>    }
> Sergey> +
> Sergey>    if (make_join_select(this, select, conds))
> Sergey>    {
> Sergey>      zero_result_cause=
> Sergey> @@ -1289,7 +1299,8 @@
> Sergey>    if (need_tmp || select_distinct || group_list || order)
> Sergey>    {
> Sergey>      for (uint i = const_tables; i < tables; i++)
> Sergey> -      join_tab[i].table->prepare_for_position();
> Sergey> +      table[i]->prepare_for_position();
> Sergey> +
> 
> Isn't table[] in other order than join_tab?
> (I thought that only join_tab has the const tables first)

Right. Will fix this.

> <cut>
> 
> Sergey> +JOIN_TAB *first_linear_tab(JOIN *join, bool after_const_tables)
> Sergey> +{
> Sergey> +  JOIN_TAB *first= join->join_tab;
> Sergey> +  if (after_const_tables)
> Sergey> +    first += join->const_tables;
> 
> remove space before '+'
> 
> Sergey> +  if (first < join->join_tab + join->top_jtrange_tables)
> Sergey> +    return first;
> Sergey> +  else
> Sergey> +    return NULL;
> Sergey> +}
> 
> Better to do:
> 
> return (first < join->join_tab + join->top_jtrange_tables) ? first : 0;
> 
> Or:
> 
> if (first < join->join_tab + join->top_jtrange_tables)
>   return first;
> return NULL;
> 
Changed.

> <cut>
> 
> Sergey> +JOIN_TAB *next_linear_tab(JOIN* join, JOIN_TAB* tab, bool include_bush_roots) //psergey2: added
> 
> Remove comments; It's trival to see that the function was added :)
> 
> Sergey> +{
> Sergey> +  if (include_bush_roots && tab->bush_children)
> Sergey> +    return tab->bush_children->start;
> Sergey> +
> Sergey> +  if (tab->last_leaf_in_bush)
> Sergey> +    tab= tab->bush_root_tab;
> Sergey> +
> Sergey> +  if (tab->bush_root_tab)
> Sergey> +    return ++tab;
> 
> Add an assert before if (tab->last_leaf_in_bush):
> 
> DBUG_ASSERT(!tab->last_leaf_in_bush || tab->bush_root_tab);
> Just to declare that the above code is safe!

Done.

> Sergey> +
> Sergey> +  if (++tab == join->join_tab + join->top_jtrange_tables /*join->join_tab_ranges.head()->end*/)
> 
> Move the comment to previous row and make it more clear what you are testing
> (The current comment doesn't tell me much)
> 
> Sergey> +    return NULL;
> Sergey> +
> Sergey> +  if (!include_bush_roots && tab->bush_children)
> Sergey> +  {
> Sergey> +    tab= tab->bush_children->start;
> Sergey> +  }
> Sergey> +  return tab;
> 
> Why not do:
> 
> return ((!include_bush_roots && tab->bush_children) ?
>          tab->bush_children->start : tab);
> 
> Sergey> +  if ((start? tab: ++tab) == join->join_tab_ranges.head()->end)
> Sergey> +    return NULL; /* End */
> 
> I think the above code would be more clear if you would do:
> 
>   if (start)
>     tab++;
>   if (...)
> 
> This makes it clear that the ++tab is not just for the test but also
> for future usage of tab.

Changed.
> Sergey> +
> Sergey> +  if (tab->bush_children)
> Sergey> +    return tab->bush_children->start;
> Sergey> +
> Sergey> +  return tab;
> 
> Could be combined with ?
> 
> Sergey> +}
> Sergey> +
> Sergey> +
> Sergey> +static Item *null_ptr= NULL;
> 
> Can we make this const, so that if anyone tried to change this memory
> location we would get an exception?

Done
> <cut>
> 
> Sergey>      DBUG_RETURN(TRUE);                        /* purecov: inspected */
>  
> Sergey>    join_tab= parent->join_tab_reexec;
> Sergey> +  //psergey2: hopefully this is ok:
> Sergey> + // join_tab_ranges.head()->start= join_tab;
> Sergey> + // join_tab_ranges.head()->end= join_tab + 1;
> 
> Better to know than to hope :)

It wasn't ok actually, already changed.
> 
> (sorry, don't have time to look at the rest now)

BR
 Sergey
-- 
Sergey Petrunia, Software Developer
Monty Program AB, http://askmonty.org
Blog: http://s.petrunia.net/blog