Commit 39b0af1e authored by Guilhem Bichot's avatar Guilhem Bichot

Fix for Bug#57932 "query with avg returns incorrect results":

when there was one NULL value, AVG(DISTINCT) could forget about other values.
See commit comment of item_sum.cc.

mysql-test/r/func_group.result:
  before the code fix, both SELECTs would return NULL
sql/item_sum.cc:
  Assume we are executing "SELECT AVG([DISTINCT] some_field) FROM some_table".
  and some_field is the single field of some_table for simplicity.
  Each time a row is processed (evaluate_join_record()->
  end_send_group()->update_sum_func()) an aggregator is notified,
  which itself notifies an Item_sum_avg.
  Without DISTINCT, this Item_sum_avg immediately increments its
  internal "sum of values" and "count of values" (the latter being
  Item_sum_avg::count). The count is incremented only if the row's value
  is not NULL (in Item_sum_avg::add()), per AVG() semantices. This row's value
  is available in args[0] of Item_sum_avg ("args[0]" stands for
  "the first argument of the item": it's an Item_field which automatically
  receives the row's value when a row is read from the table).
  bool Item_sum_avg::add()
  {
    if (Item_sum_sum::add()) << calculates the sum (ignores NULL)
      return TRUE;
    if (!args[0]->null_value)<<if added value is not NULL
      count++;       <<increment "count"
    return FALSE;
  }
  and everything works.
  With DISTINCT, when a row is processed by evaluate_join_record(),
  Item_sum_avg does no immediate computation, rather stores
  the row's value in a tree (to throw the value away if it is a duplicate
  of previous value, otherwise to remember all
  distinct values). It's only when it's time to send the average to the
  user (at end of the query:
  sub_select(end_of_records=true)->end_send_group()->
  select_send->send_data()->Protocol::send_result_set_row()->
  Item::send()->Item_sum_avg->val_str()), that we iterate over the tree,
  compute the sum and count: for this, for each element of the tree,
  Item_sum_avg::add() is called and has the same two steps as before:
  * Item_sum_sum::add() updates the sum (finding the tree element's value
  correctly, and determining correctly its NULLness - look for "arg_is_null"
  in that function)
  * the "if (!args[0]->null_value)" test right after, breaks: it uses args[0],
  which isn't the tree's element but rather the value for the last row
  processed by evaluate_join_record(). So if that last row was NULL,
  "count" stays 0 for each row, and AVG() then returns NULL (count==0 =>
  NULL, per AVG() semantics).
  The fix is to let the aggregator tell whether the value
  it just saw was NULL. The aggregator knows where to get the info
  thanks to virtual functions. Item_sum_sum::add() now asks
  the aggregator. Item_sum_avg() also asks the aggregator
  and then knows it shouldn't increment "count".
sql/item_sum.h:
  Aggregator can now tell about value/NULLness of just-aggregated value
parent 209df6a6
...@@ -1746,3 +1746,18 @@ MAX(c1) MIN(c1) ...@@ -1746,3 +1746,18 @@ MAX(c1) MIN(c1)
-00:00:01 -00:00:01 -00:00:01 -00:00:01
DROP TABLE t1; DROP TABLE t1;
# End of the bug#56120 # End of the bug#56120
#
# Bug#57932 "query with AVG(DISTINCT) returns NULL if last
# aggregated value was NULL"
#
CREATE TABLE t1 (col_int_nokey int(11));
INSERT INTO t1 VALUES (7),(8),(NULL);
SELECT AVG(DISTINCT col_int_nokey) FROM t1;
AVG(DISTINCT col_int_nokey)
7.5000
SELECT AVG(DISTINCT outr.col_int_nokey) FROM t1 AS outr LEFT JOIN t1 AS outr2 ON
outr.col_int_nokey = outr2.col_int_nokey;
AVG(DISTINCT outr.col_int_nokey)
7.5000
DROP TABLE t1;
# End of the bug#57932
...@@ -1118,3 +1118,14 @@ SELECT MAX(c1),MIN(c1) FROM t1; ...@@ -1118,3 +1118,14 @@ SELECT MAX(c1),MIN(c1) FROM t1;
DROP TABLE t1; DROP TABLE t1;
--echo # End of the bug#56120 --echo # End of the bug#56120
--echo #
--echo # Bug#57932 "query with AVG(DISTINCT) returns NULL if last
--echo # aggregated value was NULL"
--echo #
CREATE TABLE t1 (col_int_nokey int(11));
INSERT INTO t1 VALUES (7),(8),(NULL);
SELECT AVG(DISTINCT col_int_nokey) FROM t1;
SELECT AVG(DISTINCT outr.col_int_nokey) FROM t1 AS outr LEFT JOIN t1 AS outr2 ON
outr.col_int_nokey = outr2.col_int_nokey;
DROP TABLE t1;
--echo # End of the bug#57932
...@@ -1325,27 +1325,11 @@ void Item_sum_sum::fix_length_and_dec() ...@@ -1325,27 +1325,11 @@ void Item_sum_sum::fix_length_and_dec()
bool Item_sum_sum::add() bool Item_sum_sum::add()
{ {
DBUG_ENTER("Item_sum_sum::add"); DBUG_ENTER("Item_sum_sum::add");
bool arg_is_null;
if (hybrid_type == DECIMAL_RESULT) if (hybrid_type == DECIMAL_RESULT)
{ {
my_decimal value, *val; my_decimal value;
if (aggr->use_distinct_values) const my_decimal *val= aggr->arg_val_decimal(&value);
{ if (!aggr->arg_is_null())
/*
We are aggregating distinct rows. Get the value from the distinct
table pointer
*/
Aggregator_distinct *daggr= (Aggregator_distinct *)aggr;
val= daggr->table->field[0]->val_decimal (&value);
arg_is_null= daggr->table->field[0]->is_null();
}
else
{
/* non-distinct aggregation */
val= args[0]->val_decimal(&value);
arg_is_null= args[0]->null_value;
}
if (!arg_is_null)
{ {
my_decimal_add(E_DEC_FATAL_ERROR, dec_buffs + (curr_dec_buff^1), my_decimal_add(E_DEC_FATAL_ERROR, dec_buffs + (curr_dec_buff^1),
val, dec_buffs + curr_dec_buff); val, dec_buffs + curr_dec_buff);
...@@ -1355,25 +1339,8 @@ bool Item_sum_sum::add() ...@@ -1355,25 +1339,8 @@ bool Item_sum_sum::add()
} }
else else
{ {
double val; sum+= aggr->arg_val_real();
if (aggr->use_distinct_values) if (!aggr->arg_is_null())
{
/*
We are aggregating distinct rows. Get the value from the distinct
table pointer
*/
Aggregator_distinct *daggr= (Aggregator_distinct *)aggr;
val= daggr->table->field[0]->val_real ();
arg_is_null= daggr->table->field[0]->is_null();
}
else
{
/* non-distinct aggregation */
val= args[0]->val_real();
arg_is_null= args[0]->null_value;
}
sum+= val;
if (!arg_is_null)
null_value= 0; null_value= 0;
} }
DBUG_RETURN(0); DBUG_RETURN(0);
...@@ -1471,6 +1438,45 @@ Aggregator_distinct::~Aggregator_distinct() ...@@ -1471,6 +1438,45 @@ Aggregator_distinct::~Aggregator_distinct()
} }
my_decimal *Aggregator_simple::arg_val_decimal(my_decimal *value)
{
return item_sum->args[0]->val_decimal(value);
}
double Aggregator_simple::arg_val_real()
{
return item_sum->args[0]->val_real();
}
bool Aggregator_simple::arg_is_null()
{
return item_sum->args[0]->null_value;
}
my_decimal *Aggregator_distinct::arg_val_decimal(my_decimal * value)
{
return use_distinct_values ? table->field[0]->val_decimal(value) :
item_sum->args[0]->val_decimal(value);
}
double Aggregator_distinct::arg_val_real()
{
return use_distinct_values ? table->field[0]->val_real() :
item_sum->args[0]->val_real();
}
bool Aggregator_distinct::arg_is_null()
{
return use_distinct_values ? table->field[0]->is_null() :
item_sum->args[0]->null_value;
}
Item *Item_sum_count::copy_or_same(THD* thd) Item *Item_sum_count::copy_or_same(THD* thd)
{ {
return new (thd->mem_root) Item_sum_count(thd, this); return new (thd->mem_root) Item_sum_count(thd, this);
...@@ -1576,7 +1582,7 @@ bool Item_sum_avg::add() ...@@ -1576,7 +1582,7 @@ bool Item_sum_avg::add()
{ {
if (Item_sum_sum::add()) if (Item_sum_sum::add())
return TRUE; return TRUE;
if (!args[0]->null_value) if (!aggr->arg_is_null())
count++; count++;
return FALSE; return FALSE;
} }
......
...@@ -101,6 +101,15 @@ public: ...@@ -101,6 +101,15 @@ public:
*/ */
virtual void endup() = 0; virtual void endup() = 0;
/** Decimal value of being-aggregated argument */
virtual my_decimal *arg_val_decimal(my_decimal * value) = 0;
/** Floating point value of being-aggregated argument */
virtual double arg_val_real() = 0;
/**
NULLness of being-aggregated argument; can be called only after
arg_val_decimal() or arg_val_real().
*/
virtual bool arg_is_null() = 0;
}; };
...@@ -304,6 +313,7 @@ class st_select_lex; ...@@ -304,6 +313,7 @@ class st_select_lex;
class Item_sum :public Item_result_field class Item_sum :public Item_result_field
{ {
friend class Aggregator_distinct; friend class Aggregator_distinct;
friend class Aggregator_simple;
protected: protected:
/** /**
...@@ -600,6 +610,9 @@ public: ...@@ -600,6 +610,9 @@ public:
void clear(); void clear();
bool add(); bool add();
void endup(); void endup();
virtual my_decimal *arg_val_decimal(my_decimal * value);
virtual double arg_val_real();
virtual bool arg_is_null();
bool unique_walk_function(void *element); bool unique_walk_function(void *element);
static int composite_key_cmp(void* arg, uchar* key1, uchar* key2); static int composite_key_cmp(void* arg, uchar* key1, uchar* key2);
...@@ -623,6 +636,9 @@ public: ...@@ -623,6 +636,9 @@ public:
void clear() { item_sum->clear(); } void clear() { item_sum->clear(); }
bool add() { return item_sum->add(); } bool add() { return item_sum->add(); }
void endup() {}; void endup() {};
virtual my_decimal *arg_val_decimal(my_decimal * value);
virtual double arg_val_real();
virtual bool arg_is_null();
}; };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment