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?
