From: Martin Hansson
Date: February 15 2011 2:20pm
Subject: Re: bzr commit into mysql-5.1 branch (martin.hansson:3587) Bug#59839
List-Archive: http://lists.mysql.com/commits/131340
Message-Id: <4D5A8BA6.4090701@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi! Thank you for the fast response! I feel obliged to return the favor!

Roy Lyseng skrev 2011-02-15 15.07:
> ...
>>>> +SELECT a, AVG(t1.b),
>>>> +(SELECT t11.c FROM t1 t11 WHERE t11.a = t1.a AND t11.b = 
>>>> AVG(t1.b)) AS t11c,
>>>> +(SELECT t12.c FROM t1 t12 WHERE t12.a = t1.a AND t12.b = 
>>>> AVG(t1.b)) AS t12c
>>>> +FROM t1 GROUP BY a;
>>>
>>> I'd suggest that you write something like:
>>> set $query=SELECT ...;
>>> eval EXPLAIN $query;
>>> eval $query;
>> Well personally I don't like that style and haven't seen anybody else 
>> using it.
>
>
> ...but you did not challenge my argument :)
Nope, and that's on purpose. :-) My only argument is taste. It's more 
writing and you are forced to break lines after less SQL, which I don't 
like.
>
> Anyway, this was just a suggestion.
>
>> If anything, we should adopt the new explain mode in mysql_test_run.
>
> I'll wait to see how that mode turns out. Is it maintainable?
You might ask Tor about it, haven't heard anything in a long time.
>>>> + Item_direct_ref)
>>>> +
>>>> + - fixes references (Item_ref objects) to these fields.
>>> fields -> items
>> Not sure about that one. Can you prove it isn't always fields?
>
> Nope, this was more of a wild guess :D
And besides you already approved the patch so I can do as I please. 
Haha! :-)
>
>>> @return true if an error occurred, false if success.
>> Us humans can understand that just fine, but poor Doxygen can't.
> This return convention is so common, that I would advice you to write:
>
> Personally, I do not care that much about what Doxygen might think...
Yes, it seems I'm the only one poking around in the Doxygen pages :')
>>>> +
>>>> + @todo This comment must be restructured. It is jumping back and 
>>>> forth
>>>> + between abstraction levels and it uses inconsistent terminology.
>>>
>>> Hey! There is actual documentation here! You can either remove the 
>>> @todo, fix
>>> the comment, or pinpoint the jumping and use of inconsistent 
>>> terminology.
>> My apologies, didn't mean to offend anyone. I realize that this 
>> function has
>> been gradually edited from its initial appearance. In order to make 
>> amends I'll
>> try to do all three!
>>
>> It looks like the original author was using the term "Sum" instead of 
>> aggregate.
>> And someone (perhaps you) had been kind enough to change it to 
>> "aggregate" in
>> most places, so I fixed the rest of them.
>>
>> So I try leaving it in a little better state than I found it. But I 
>> think making
>> the comment 100% correct is beyond a bug fix.
>>
>> In my defense I'll add that this line was not meant to go in the 
>> patch. It was
>> more like a memento for me that kinda slipped by.
>>>
>>> I guess the inconsistet terminology issues are: field vs. item. 
>>> select vs
>>> query block (I prefer query specification or query expression, they 
>>> are SQL
>>> official terms),
>> Yes, and sum versus aggregate, fix versus resolve, and what does 
>> "pull up" mean?
>>
>> And the comment is jumping back and forth between SQL terminology, 
>> high-level
>> design and class layout and list structures, etc.
>
> Nevertheless, the description is almost spot-on.
I take your word for it :-)
>>
>> But it isn't as simple as search-and-replace either. For instance I 
>> suspect that
>> "fix" means different things in different sentences.
>
> It goes without saying. You cannot fix an inconsistent description by 
> applying regular principles.
Well if it's as simple as replacing "sum" with "aggregate" and "fix" 
with "resolve" you can...which is basically what I did...
>>>> + */
>>>>
>>>> bool
>>>> fix_inner_refs(THD *thd, List<Item> &all_fields, SELECT_LEX *select,
>>>> @@ -328,11 +336,11 @@ fix_inner_refs(THD *thd, List<Item> &all
>>>> {
>>>> Item_outer_ref *ref;
>>>> bool res= FALSE;
>>> The local variable res is unused and can be removed.
>> Not the bug strictly speaking, but since it gives me an excuse to 
>> change FALSE
>> to false I'll add it in. :-)
>
> This is the kind of minor, local refactoring I think that we can apply 
> as part of regular bug fixing. It has (almost) zero risk, and is not 
> easy to confuse with the actual bug fix.
I agree. Igor would disagree, but he's not here anymore is he? ;-)

Best Regards

Martin