From: Roy Lyseng
Date: January 24 2011 3:50pm
Subject: Re: bzr commit into mysql-trunk branch (tor.didriksen:3328)
List-Archive: http://lists.mysql.com/commits/129462
Message-Id: <4D3D9FB0.7000904@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Thank you for the review.

On 21.01.11 13.51, Tor Didriksen wrote:
> #At file:///export/home/didrik/repo/next-mr-opt-backporting-bug57623/ based on revid:tor.didriksen@stripped
>
>   3328 Tor Didriksen	2011-01-21
>        review comments
>
>      modified:
>        sql/sql_select.cc
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2011-01-21 12:50:36 +0000
> +++ b/sql/sql_select.cc	2011-01-21 12:51:45 +0000
> @@ -4139,9 +4139,12 @@ skip_conversion:
>
>       if (!thd->stmt_arena->is_conventional())
>       {
> -      tree= ((*subq)->embedding_join_nest == (TABLE_LIST*)1)?
> -&select_lex->prep_where :
> -&((*subq)->embedding_join_nest->prep_on_expr);
> +      const bool have_join_nest=
> +        (*subq)->embedding_join_nest != (TABLE_LIST*) 1;
> +
> +      tree= have_join_nest ?
> +&((*subq)->embedding_join_nest->prep_on_expr) :
> +&select_lex->prep_where;

OK, but "subquery_in_join_clause" may be a better name than have_join_nest.
>
>         /*
>           Some precaution is needed when dealing with PS/SP:
> @@ -4150,9 +4153,17 @@ skip_conversion:
>           simplify_joins(), which is called after this function. Hence, we need
>           to check that *tree is non-NULL before calling replace_subcondition.
>         */
> +      /*
> +       I don't understand what you are trying to say here.
> +       Could you re-write to something like
> +       if (have_join_nest)
> +         DEBUG_ASSERT(something);
> +       else
> +         DEBUG_ASSERT(something else);
> +      */

I dislike adding extra code outside of an assert. I prefer to put everything 
inside it. I guess a clever optimizer will remove that code, but nevertheless I 
prefer this style.
>         DBUG_ASSERT(((*subq)->embedding_join_nest == (TABLE_LIST*)1 ||
>                      (*subq)->embedding_join_nest->nested_join == NULL) ==
> -                  (*tree != 0));
> +                  (*tree != NULL));

The debug assert means: If subquery within WHERE clause then OK, otherwise if 
the join nest represents a single table, prep_on_expr must be non-NULL, 
otherwise if the join nest represents a joined table, prep_on_expr must be NULL.
>         if (*tree&&  replace_subcondition(this, tree, *subq, substitute, FALSE))
>           DBUG_RETURN(TRUE);
>       }

Thanks,
Roy