maria-developers team mailing list archive

[Igor Babaev <igor@xxxxxxxxxxxx>]

On 03/25/2013 09:59 AM, Sergei Petrunia wrote:
> An alternative patch:
> 
> === modified file 'sql/item_sum.cc'
> --- sql/item_sum.cc     2013-02-28 17:42:49 +0000
> +++ sql/item_sum.cc     2013-03-25 16:15:05 +0000
> @@ -1539,7 +1539,8 @@
>  
>  bool Item_sum_count::add()
>  {
> -  if (!args[0]->maybe_null || !args[0]->is_null())
> +  //if (!args[0]->maybe_null || !args[0]->is_null())
> +  if (!aggr->arg_is_null())
>      count++;
>    return 0;
>  }

Honestly, I did not check it.

Yet:
With this code you still have unnecessary copying of
the elements themselves.

Besides:
For COUNT(DISTINCT ...) we already have a separate branch
when Unique fits the allocated memory:
to get the count in this case we just take it from the
RB tree of the Unique object, bypassing the tree retrieval.

Regards,
Igor.

> 
> 
> Any reason this will not work?
> 
> On Sun, Mar 24, 2013 at 10:20:41AM -0700, Igor Babaev wrote:
>> 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
>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~maria-developers
>> Post to     : maria-developers@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~maria-developers
>> More help   : https://help.launchpad.net/ListHelp
>