From: Jorgen Loland
Date: May 6 2011 1:20pm
Subject: Re: bzr commit into mysql-trunk branch (jorgen.loland:3340) Bug#11765831
List-Archive: http://lists.mysql.com/commits/136835
Message-Id: <4DC3F599.6060308@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Hi Olav,

Thank you for the review. Unless commented on below, I have incorporated your 
exact suggestions

On 05/06/2011 11:25 AM, Olav Sandstaa wrote:
> Hi Jørgen,
>
> Thanks for making this effort on adding systematic comments to the key_or()
> function. I like your "ascii art" comments. This is makes it much easier to
> understand what and where things are done.
>
> I have only very minor comments. OK to push this patch after you have looked at
> my comments.
>
> Olav
>
> + /*
> + Used to describe how two key values are positioned compared to
> + each other. Consider key_value_a.<cmp_func>(key_value_b):
> +
> + -2: key_value_a is smaller than key_value_b, and they are adjacent
> + -1: key_value_a is smaller than key_value_b (not adjacent)
> + 0: the key values are equal
> + 1: key_value_a is bigger than key_value_b (not adjacent)
> + -2: key_value_a is bigger than key_value_b, and they are adjacent
> +
> + Example: "cmp= tmp->cmp_max_to_min(key2)"
> +
> + key2: [-------- (10 <= x ...)
> + tmp: -----] (... x < 10) => cmp==-2
> + tmp: ----] (... x <= 9) => cmp==-1
> + tmp: ------] (... x = 10) => cmp== 0
> + tmp: --------] (... x <= 12) => cmp== 1
>
> Great explanation. For completeness maybe include also an example for cmp==2?

I would have to change to another compare function then. cmp == 2 does not make 
sense for cmp_max_to_min()

> + /*
> + This is the case:
> + key2: [-------]
> + tmp: [----]
> + */
> SEL_ARG *next=tmp->next;
> - /* key1 on the left of key2 non-overlapping */
>
> Consider including this comment in the above (new comment). I found it useful
> since it included the word "key1" instead of just tmp.

I added this instead:

     /*
+     key1 consists of one or more ranges. tmp is the range currently
+     being handled.
+
       initialize tmp to the latest range in key1 that starts the same
       place or before the range in key2 starts

       key2:           [------]
       key1: [---] [-----] [----]
                   ^
                   tmp
     */
     SEL_ARG *tmp=key1->find_range(key2);


-- 
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway