Re: [Maria-developers] Rev 3701: Fixed bug mdev-4311
Hi, Igor! On Mar 22, Igor Babaev wrote:
At file:///home/igor/maria/maria-5.5-bug4311/
------------------------------------------------------------ revno: 3701 revision-id: igor@askmonty.org-20130322224651-r2tpn9in5i1ifv7a parent: sergii@pisem.net-20130317104125-yyp99euwpir5ueho committer: Igor Babaev <igor@askmonty.org> 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. 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
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@askmonty.org-20130322224651-r2tpn9in5i1ifv7a parent: sergii@pisem.net-20130317104125-yyp99euwpir5ueho committer: Igor Babaev <igor@askmonty.org> 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
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; } 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@askmonty.org-20130322224651-r2tpn9in5i1ifv7a parent: sergii@pisem.net-20130317104125-yyp99euwpir5ueho committer: Igor Babaev <igor@askmonty.org> 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@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
-- BR Sergei -- Sergei Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog
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@askmonty.org-20130322224651-r2tpn9in5i1ifv7a parent: sergii@pisem.net-20130317104125-yyp99euwpir5ueho committer: Igor Babaev <igor@askmonty.org> 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@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
No, this one causes failures.. I'll try a bit more to get another one.. On Mon, Mar 25, 2013 at 08:59:40PM +0400, 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; }
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@askmonty.org-20130322224651-r2tpn9in5i1ifv7a parent: sergii@pisem.net-20130317104125-yyp99euwpir5ueho committer: Igor Babaev <igor@askmonty.org> 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@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
-- BR Sergei -- Sergei Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
-- BR Sergei -- Sergei Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog
participants (3)
-
Igor Babaev -
Sergei Golubchik -
Sergei Petrunia