MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Jørgen Løland Date:March 1 2010 12:36pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3354)
Bug#47762
View as plain text  
Martin,

Patch approved with minor comments (please see comments inline).

Martin Hansson wrote:
> #At file:///data0/martin/bzr/bug47762/5.1bt/ based on
> revid:magne.mahre@stripped
> 
>  3354 Martin Hansson	2010-02-25
>       Bug#47762: Incorrect result from MIN() when WHERE tests NOT
>       NULL column for NULL
>       
>       The optimization to read MIN() and MAX() values from an
>       index did not properly handle comparisons with NULL
>       values. Fixed by giving up the particular optimization step
>       if there are non-NULL safe comparisons with NULL values, as 
>       the result is NULL anyway.
>      @ sql/field.h
>         Bug#47762: Only commenting
> 
>     added:
>       mysql-test/include/min_null_cond.inc
>     modified:
>       mysql-test/r/group_min_max.result
>       mysql-test/t/group_min_max.test
>       sql/field.h
>       sql/opt_sum.cc

According to our new policies, all these files must be updated with the 
Oracle copyright notice.

> === modified file 'sql/opt_sum.cc'
> --- a/sql/opt_sum.cc	2010-02-09 08:53:13 +0000
> +++ b/sql/opt_sum.cc	2010-02-25 10:42:43 +0000
> @@ -695,9 +708,15 @@ static bool matching_cond(bool max_fl, T
>      *key_part_used|= (key_part_map) 1 << (part - keyinfo->key_part);
>    }
>  
> +  /*
> +    We pre-evaluate the condition on the keypart. Calls to is_null() below
> +    depend on this for some Item's. This way we also avoid getting an if-test
> +    with side effects.
> +  */
> +  bool possibly_null_valued= cond->val_int() == 0;

I'm not quite comfortable with this var name. What about 
key_may_be_null? I'll leave the decision to you.

>    if (org_key_part_used != *key_part_used ||
>        (is_field_part && 
> -       (between || eq_type || max_fl == less_fl) && !cond->val_int()))
> +       (between || eq_type || max_fl == less_fl) && possibly_null_valued))
>    {
>      /*
>        It's the first predicate for this part or a predicate of the
> @@ -715,6 +734,9 @@ static bool matching_cond(bool max_fl, T
>      }
>      else
>      {
> +      Item *value= args[between && max_fl ? 2 : 1];
> +      if (value->is_null())
> +        return FALSE;

I'd prefer it if you reused the "between && ..." evaluation.

>        store_val_in_field(part->field, args[between && max_fl ? 2 : 1],
>                           CHECK_FIELD_IGNORE);
>        if (part->null_bit) 
> @@ -737,13 +759,19 @@ static bool matching_cond(bool max_fl, T
>    }
>    else if (eq_type)
>    {
> -    if ((!is_null && !cond->val_int()) ||
> +    if ((!is_null && possibly_null_valued) ||
>          (is_null && !test(part->field->is_null())))
>       return 0;                       // Impossible test
>    }
>    else if (is_field_part)
>      *range_fl&= ~(max_fl ? NO_MIN_RANGE : NO_MAX_RANGE);
> -  return 1;  
> +
> +  if (between && (args[1]->is_null() || args[2]->is_null()))
> +    return FALSE;
> +  if (!is_null && args[1]->is_null())
> +    return FALSE;
> +
> +  return TRUE;
>  }


-- 
Jørgen Løland
Thread
bzr commit into mysql-5.1-bugteam branch (martin.hansson:3354) Bug#47762Martin Hansson25 Feb
Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3354)Bug#47762Jørgen Løland1 Mar
  • Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3354)Bug#47762Martin Hansson2 Mar