From: Roy Lyseng
Date: December 6 2010 9:35am
Subject: Re: bzr commit into mysql-5.5-bugteam branch (Georgi.Kodinov:3135)
 Bug#57954
List-Archive: http://lists.mysql.com/commits/126110
Message-Id: <4CFCAE50.7070708@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Georgi,

thank you for fixing this problem.

The bug fix is approved.

I have one suggestion for extended test coverage below, one suggestion for an 
improved comment,
and one suggestion to replace the reset_and_add() function with its contents.

All suggestions are optional, though.

On 02.12.10 15.29, Georgi Kodinov wrote:
> #At file:///Users/kgeorge/mysql/work/B57954-5.5-bugteam/ based on revid:davi.arnaut@stripped
>
>   3135 Georgi Kodinov	2010-12-02
>        Bug #57954: BIT_AND function returns incorrect results
>         when semijoin=on
>
>        When setting the aggregate function as having no rows to report
>        the function no_rows_in_result() was calling Item_sum::reset().
>        However this function in addition to cleaning up the aggregate
>        value by calling aggregator_clear() was also adding the current
>        value to the aggregate value by calling aggregator_add().
>        Fixed by making no_rows_in_result() to call aggregator_clear()
>        directly.
>        Renamed Item_sum::reset to Item_sum::reset_and_add() to
>        and added a comment to avoid misinterpretation of what the
>        function does.

Small typo "to"
>      modified:
>        mysql-test/r/func_group_innodb.result
>        mysql-test/t/func_group_innodb.test
>        sql/item_sum.cc
>        sql/item_sum.h
>        sql/opt_range.cc
>        sql/opt_sum.cc
>        sql/sql_select.cc
> === modified file 'mysql-test/r/func_group_innodb.result'
> --- a/mysql-test/r/func_group_innodb.result	2007-05-29 12:58:18 +0000
> +++ b/mysql-test/r/func_group_innodb.result	2010-12-02 14:29:45 +0000
> @@ -145,3 +145,31 @@ select count(*), min(7), max(7) from t2m
>   count(*)	min(7)	max(7)
>   0	NULL	NULL
>   drop table t1m, t1i, t2m, t2i;
> +#
> +# Bug #57954: BIT_AND function returns incorrect results when
> +#   semijoin=on
> +CREATE TABLE C (
> +pk INT,
> +col_varchar_key VARCHAR(1),
> +PRIMARY KEY (pk),
> +KEY col_varchar_key (col_varchar_key)
> +) ENGINE=InnoDB;
> +INSERT INTO C VALUES (11,NULL);
> +INSERT INTO C VALUES (16,'c');
> +CREATE TABLE BB (
> +pk INT,
> +col_varchar_key VARCHAR(1),
> +PRIMARY KEY (pk),
> +KEY col_varchar_key (col_varchar_key)
> +) ENGINE=InnoDB;
> +INSERT INTO BB VALUES (10,NULL);
> +SELECT straight_join BIT_AND(c.pk)
> +FROM
> +bb, c
> +WHERE c.col_varchar_key='ABC'
> +ORDER BY c.pk
> +;
> +BIT_AND(c.pk)
> +18446744073709551615
> +DROP TABLE C,BB;
> +End of 5.5 tests
>
> === modified file 'mysql-test/t/func_group_innodb.test'
> --- a/mysql-test/t/func_group_innodb.test	2006-05-22 11:27:58 +0000
> +++ b/mysql-test/t/func_group_innodb.test	2010-12-02 14:29:45 +0000
> @@ -83,3 +83,35 @@ explain select count(*), min(7), max(7)
>   select count(*), min(7), max(7) from t2m, t1i;
>
>   drop table t1m, t1i, t2m, t2i;
> +
> +
> +--echo #
> +--echo # Bug #57954: BIT_AND function returns incorrect results when
> +--echo #   semijoin=on
> +
> +CREATE TABLE C (
> +  pk INT,
> +  col_varchar_key VARCHAR(1),
> +  PRIMARY KEY (pk),
> +  KEY col_varchar_key (col_varchar_key)
> +) ENGINE=InnoDB;
> +INSERT INTO C VALUES (11,NULL);
> +INSERT INTO C VALUES (16,'c');
> +CREATE TABLE BB (
> +  pk INT,
> +  col_varchar_key VARCHAR(1),
> +  PRIMARY KEY (pk),
> +  KEY col_varchar_key (col_varchar_key)
> +) ENGINE=InnoDB;
> +INSERT INTO BB VALUES (10,NULL);
> +
> +SELECT straight_join BIT_AND(c.pk)
> +FROM
> +  bb, c
> +  WHERE c.col_varchar_key='ABC'
> +ORDER BY c.pk
> +;
> +
> +DROP TABLE C,BB;
> +
> +--echo End of 5.5 tests

Can you add tests for BIT_OR() and BIT_XOR() (from bug#58050) as well?

I would also prefer lowercase table names in the CREATE TABLE, DROP TABLE and 
INSERT statements.
> === modified file 'sql/item_sum.cc'
> --- a/sql/item_sum.cc	2010-09-02 13:57:59 +0000
> +++ b/sql/item_sum.cc	2010-12-02 14:29:45 +0000
> @@ -2229,7 +2229,7 @@ void Item_sum_avg::reset_field()
>
>   void Item_sum_bit::reset_field()
>   {
> -  reset();
> +  reset_and_add();
>     int8store(result_field->ptr, bits);
>   }
>
>
> === modified file 'sql/item_sum.h'
> --- a/sql/item_sum.h	2010-08-30 07:36:04 +0000
> +++ b/sql/item_sum.h	2010-12-02 14:29:45 +0000
> @@ -396,13 +396,21 @@ public:
>     Item_sum(THD *thd, Item_sum *item);
>     enum Type type() const { return SUM_FUNC_ITEM; }
>     virtual enum Sumfunctype sum_func () const=0;
> -  inline bool reset() { aggregator_clear(); return aggregator_add(); };
> +  /**
> +    Resets the aggregate to its default value and adds the value
> +    in its attribute(s) to it.
> +  */
> +  inline bool reset_and_add()
> +  {
> +    aggregator_clear();
> +    return aggregator_add();
> +  };

Suggestion:
I am not sure the function reset()/reset_and_add() is really needed. Consider 
this fragment in opt_sum.cc:

         if (!count && !outer_tables)
         {
           item_sum->aggregator_clear();
         }
         else
           item_sum->reset();

If you remove the call to reset(), the logic should be:

         item_sum->aggregator_clear();
         if (count || outer_tables)
           item_sum->aggregator_add();

which IMHO better expresses the logic.

There are a couple of places where you will need to have two function calls 
instead of one, but the logic will again be clearer.

>
>     /*
>       Called when new group is started and results are being saved in
> -    a temporary table. Similar to reset(), but must also store value in
> -    result_field. Like reset() it is supposed to reset start value to
> -    default.
> +    a temporary table. Similar to reset_and_add(), but must also
> +    store value in result_field. Like reset_and_add() it is supposed
> +    to reset start value to default.

I propose that the above section is replaced with (see also Guilhem's comments):

   Initialize the accumulator, aggregate the first value into it, and store the 
resulting value in result_field.
>       This set of methods (reult_field(), reset_field, update_field()) of
>       Item_sum is used only if quick_group is not null. Otherwise
>       copy_or_same() is used to obtain a copy of this item.
> @@ -446,7 +454,7 @@ public:
>         set_aggregator(with_distinct ?
>                        Aggregator::DISTINCT_AGGREGATOR :
>                        Aggregator::SIMPLE_AGGREGATOR);
> -    reset();
> +    aggregator_clear();
>     }
>     virtual void make_unique() { force_copy_fields= TRUE; }
>     Item *get_tmp_table_item(THD *thd);
>
> === modified file 'sql/opt_range.cc'
> --- a/sql/opt_range.cc	2010-11-19 17:15:47 +0000
> +++ b/sql/opt_range.cc	2010-12-02 14:29:45 +0000
> @@ -11391,7 +11391,7 @@ void QUICK_GROUP_MIN_MAX_SELECT::update_
>
>     min_functions_it->rewind();
>     while ((min_func= (*min_functions_it)++))
> -    min_func->reset();
> +    min_func->reset_and_add();
>   }
>
>
> @@ -11423,7 +11423,7 @@ void QUICK_GROUP_MIN_MAX_SELECT::update_
>
>     max_functions_it->rewind();
>     while ((max_func= (*max_functions_it)++))
> -    max_func->reset();
> +    max_func->reset_and_add();
>   }
>
>
>
> === modified file 'sql/opt_sum.cc'
> --- a/sql/opt_sum.cc	2010-06-11 08:15:55 +0000
> +++ b/sql/opt_sum.cc	2010-12-02 14:29:45 +0000
> @@ -424,7 +424,7 @@ int opt_sum_query(TABLE_LIST *tables, Li
>             item_sum->aggregator_clear();
>           }
>           else
> -          item_sum->reset();
> +          item_sum->reset_and_add();
>           item_sum->make_const();
>           recalc_const_item= 1;
>           break;
>
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-11-22 09:13:46 +0000
> +++ b/sql/sql_select.cc	2010-12-02 14:29:45 +0000
> @@ -15838,7 +15838,7 @@ init_sum_functions(Item_sum **func_ptr,
>   {
>     for (; func_ptr != end_ptr ;func_ptr++)
>     {
> -    if ((*func_ptr)->reset())
> +    if ((*func_ptr)->reset_and_add())
>         return 1;
>     }
>     /* If rollup, calculate the upper sum levels */

Thanks,
Roy