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 <[email protected]> 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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
