maria-developers team mailing list archive

[Igor Babaev <igor@xxxxxxxxxxxx>]

On 03/24/2013 08:37 AM, Sergei Golubchik wrote:
> Hi, Igor!
> 
> On Mar 22, Igor Babaev wrote:
>> At file:///home/igor/maria/maria-5.5-bug4311/
>>
>> ------------------------------------------------------------
>> revno: 3701
>> revision-id: igor@xxxxxxxxxxxx-20130322224651-r2tpn9in5i1ifv7a
>> parent: sergii@xxxxxxxxx-20130317104125-yyp99euwpir5ueho
>> committer: Igor Babaev <igor@xxxxxxxxxxxx>
>> branch nick: maria-5.5-bug4311
>> timestamp: Fri 2013-03-22 15:46:51 -0700
>> message:
>>   Fixed bug mdev-4311 (bug #68749).
>>   This bug was introduced by the patch for WL#3220.
>>   If the memory allocated for the tree to store unique elements
>>   to be counted is not big enough to include all of them then
>>   an external file is used to store the elements.
>>   The unique elements are guaranteed not to be nulls. So, when 
>>   reading them from the file we don't have to care about the null
>>   flags of the read values. However, we should remove the flag 
>>   at the very beginning of the process. If we don't do it and
>>   if the last value written into the record buffer for the field
>>   whose distinct values needs to be counted happens to be null,
>>   then all values read from the file  are considered to be nulls
>>   and are not counted in.
>>   The fix does not remove a possible null flag for the read values.
>>   Rather it just counts the values in the same way it was done
>>   before WL #3220.
> 
>> === modified file 'sql/item_sum.cc'
>> --- a/sql/item_sum.cc	2013-02-28 17:42:49 +0000
>> +++ b/sql/item_sum.cc	2013-03-22 22:46:51 +0000
>> @@ -1460,6 +1460,12 @@
>>    
>>  bool Aggregator_distinct::unique_walk_function(void *element)
>>  {
>> +  if (item_sum->sum_func() == Item_sum::COUNT_DISTINCT_FUNC)
>> +  {
>> +    Item_sum_count *sum= (Item_sum_count *)item_sum;
>> +    sum->count++;
>> +    return 0;
>> +  }
>>    memcpy(table->field[0]->ptr, element, tree_key_length);
>>    item_sum->add();
>>    return 0;
> 
> I'm not sure I like it. You've added a special check for
> COUNT_DISTINCT_FUNC just before the call of item_sum->add() which is
> virtual, and Item_sum_count has its own implementation of it. Logically,
> if there's a virtual method, the check should be in there, not in the
> caller.

I don't see any problem here because
Aggregator_distinct::unique_walk_function
is not virtual.
Actually here we have two 'implementations' of the
Aggregator_distinct::unique_walk_function processor.
One of them is used only for COUNT(DISTINCT) items.

Currently Sergey Petrunia is already fighting with unnecessary virtual
functions in these classes. I remove one of them, that is good.

Regards,
Igor.

> 
> And what about SUM(DISTINCT ...) ?
> 
> Perhaps it's better to mark the item not null, as you've explained in
> the changeset comment?
> 
> Regards,
> Sergei