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@askmonty.org> 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