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
