List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:April 11 2011 11:41am
Subject:Re: bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517
View as plain text  
On 04/11/11 01:31 PM, Roy Lyseng wrote:
> Hi Øystein,
> On 11.04.11 11.36, Øystein Grøvlen wrote:
>> Hi Roy,
>> I think changing the meaning of remaining_tables is very good since it
>> makes the
>> interface more general. This is also illustrated by the patch since
>> you are able
>> to remove semi-join specific code from best_extension_by_limited
>> search().
>> AFAICT, you should also be able to simplify the code that compute the
>> constant
>> n_tables in greedy_search(), and I suggest you include that in this
>> patch.
> It seems that I lost that part when splitting the patch in two. Thank
> you for catching.
>> However, you are also adding more semi-join dependent code into
>> choose_table_order(), and I do not think that is a good idea. In my
>> opinion,
>> excluded_tables and join_tables are general concepts that should
>> rather be
>> computed by the caller, which knows the context, and passed as
>> parameters. The
>> same also holds for jtab_sort_func. This will, in my opinion, give a
>> better
>> separation of concerns.
> I think there are arguments for both approaches. In either case, I think
> that it is impossible to remove semijoin-related handling from
> greedy_search. Actually, I fear that we need to add more code, in
> addition to what is already in advance_sj_state() and other functions.
> I do think that the encapsulation approach is good: The new class is fed
> with a JOIN pointer and a semi-join nest pointer (which may be NULL),
> and the class calculates the additional information needed from these
> classes.
> Now, there are exactly two places where this class is called, one with a
> non-NULL and another with a NULL argument to emb_sjm_nest. Thus, it is
> possible to calculate information derived from emb_sjm_nest externally
> in exactly one place, and thus avoid the if test inside
> choose_table_order(). Nevertheless, I think that it is good to
> encapsulate this calculation also, and thus hide it from the caller. But
> this is probably a matter of taste.

I still do not agree, but I will not insist.

>> See also inline for some comments to the commit comments.
>> On 04/ 6/11 03:27 PM, Roy Lyseng wrote:
>>> #At file:///home/rl136806/mysql/repo/mysql-work5/ based on
>>> revid:roy.lyseng@stripped
>>> 3359 Roy Lyseng 2011-04-06
>>> Bug#11822517: Refactor call interface of choose_plan()
>>> Part 2 - Change meaning of greedy_search() argument 'remaining_tables'.
>>> The purpose of this patch is to make the interface of greedy_search()
>>> and related functions simpler.
>>> It is also a preparatory step for a simpler test for dependencies when
>>> doing semi join materialization together with outer join (WL#5561).
>>> The meaning of the argument remaining_tables to greedy_search() is
>>> changed:
>>> Before, it meant all tables not added to the current join plan,
>>> regardless of whether the tables were to be optimized or not.
>>> Now, it means all tables remaining to be added.
>> I think the last sentence needs to be qualified with "by this. The way
>> it since
>> now, I do not see what has changes since what is described in the
>> previous
>> sentence.
> I am not sure that I understand what you mean.

Sorry, my first sentence was not complete.  I think the comments needs 
to more explicitly distinguish between the task of optimizing the whole 
query, and the subtask of optimizing a join nest.  I assume "current 
join plan" refers to the latter, and that the last sentence is written 
in that context, but I am not quite sure.


bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517Roy Lyseng6 Apr
  • Re: bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517Guilhem Bichot8 Apr
  • Re: bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517Øystein Grøvlen11 Apr
    • Re: bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517Roy Lyseng11 Apr
      • Re: bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517Øystein Grøvlen11 Apr