List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:December 1 2010 1:36pm
Subject:Re: #57932 [Com]: query with AVG(DISTINCT) returns NULL if last aggregated
value was NULL
View as plain text  
Hello,

Bug Database a écrit, Le 01.12.2010 13:38:
> Guilhem,
> 
> Why not add this as members of the aggregator ? 
> e.g. 
> Aggregator_distinct::arg_val_decimal(my_decimal * value)
> {
>   if (use_distinct_values)
>     return table->field[0]->val_decimal (value);
>   else
>    return item_sum->args[0]->val_decimal(value);
> }
> 
> Aggregator_distinct::arg_val_real()
> {
>   if (use_distinct_values)
>     return table->field[0]->val_real ();
>   else
>    return item_sum->args[0]->val_real();
> }
> 
> Aggregator_distinct::arg_is_null()
> {
>   if (use_distinct_values)
>     return  table->field[0]->is_null()  
>   else
>    return item_sum->args[0]->null_value;
> }
> 
> And add the corresponding members in Aggregator and
> Aggregator_simple(). 
> Maybe we can even make use_distinct_values private ?
> 
> In this way we can then remove some if()s from Item_sum_sum

It is true that Item_sum_sum::add() would be shortened.

If I understand well, we would have this code in
Item_sum_avg::add()
{
   Item_sum_sum::add();
   if (!aggr->arg_is_null()) count++;
}
And Item_sum_sum::add() would also have calls to aggr->arg_is_null():
   if (!aggr->arg_is_null()) include it in the sum;

But I find it a bit inefficient to have repeated logic: one test of NULL 
to decide whether the added number should affect the sum, and one test 
of NULL to decide whether the added number should affect the count. 
Whereas in the first patch which I proposed (where "count++" is moved to 
Item_sum_sum::add()), we get both operations in one test (the patch 
doesn't add any if(), it squeezes count++ inside an existing if()).

We could imagine a mix of the two ideas: "count" moves to Item_sum_sum 
and is maintained in Item_sum_sum::add() (idea of my first patch, no new 
if()), and we introduce Aggregator_distinct::arg_val_decimal/etc to 
simplify Item_sum_sum::add(). The best of both worlds.

What do you think of this?
Thread
Re: #57932 [Com]: query with AVG(DISTINCT) returns NULL if last aggregatedvalue was NULLGuilhem Bichot1 Dec
Re: #57932 [Com]: query with AVG(DISTINCT) returns NULL if last aggregatedvalue was NULLGuilhem Bichot1 Dec