From: Jorgen Loland
Date: February 11 2011 7:45am
Subject: Re: bzr commit into mysql-trunk branch (ole.john.aske:3622) Bug#57601
List-Archive: http://lists.mysql.com/commits/131102
Message-Id: <4D54E917.2090600@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Hi Ole John,

Thank you for the patch. I have only one real concern as can be seen inline.

> -void QUICK_RANGE_SELECT::need_sorted_output()
> +void QUICK_RANGE_SELECT::need_sorted_output(bool sort)
>   {
> -  if (!(mrr_flags&  HA_MRR_SORTED))
> +  if (sort)
>     {
> -    /*
> -      Native implementation can't produce sorted output. We'll have to
> -      switch to default
> -    */
> -    mrr_flags |= HA_MRR_USE_DEFAULT_IMPL;
> +    if (!(mrr_flags&  HA_MRR_SORTED))
> +    {
> +      /*
> +        Native implementation can't produce sorted output. We'll have to
> +        switch to default
> +      */
> +      mrr_flags |= HA_MRR_USE_DEFAULT_IMPL;
> +    }
> +    mrr_flags |= HA_MRR_SORTED;
> +  }
> +  else
> +  {
> +    mrr_flags&= ~HA_MRR_SORTED;
>     }
> -  mrr_flags |= HA_MRR_SORTED;
>   }

If need_sorted_output(false) is called after need_sorted_output(true), mrr_flags 
is incorrectly tagged with HA_MRR_USE_DEFAULT_IMPL.

> @@ -10976,7 +10994,11 @@ int QUICK_GROUP_MIN_MAX_SELECT::reset(vo
>     DBUG_ENTER("QUICK_GROUP_MIN_MAX_SELECT::reset");
>
>     head->set_keyread(TRUE); /* We need only the key attributes */
> -  if ((result= file->ha_index_init(index,1)))
> +
> +  // Request ordered index access as usage of ::index_last(), ::index_first()
> +  // within QUICK_GROUP_MIN_MAX_SELECT depends on it.

Multi-line comments should have the format
/*
  comment
*/

> +  bool sorted= true;
> +  if ((result= file->ha_index_init(index,sorted)))

It's an improvement to add the comment and change from 1 to true, but IMO it's a 
bit overkill to create a variable since it's only used once.

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