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
