maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Varun,

> revision-id: c408c41230c4a9de0114d13aaf861d3ced65f4c7 (mariadb-10.5.0-424-gc408c41230c)
> parent(s): 95da2113a050ad739fdaf60ee871329468a01554
> author: Varun Gupta
> committer: Varun Gupta
> timestamp: 2020-03-31 11:57:37 +0530
> message:
> 
> MDEV-11563: GROUP_CONCAT(DISTINCT ...) may produce a non-distinct list
> 

The commit message should describe the error and the fix made.
(especially important here as this patch fixes only a part of the issue 
with GROUP_CONCAT)

> ---
> mysql-test/main/func_gconcat.result | 53 ++++++++++++++++++++++---------------
> mysql-test/main/func_gconcat.test   |  7 +++++
> sql/item_sum.cc                     | 40 +++++++++++++++++-----------
> sql/item_sum.h                      |  3 ++-
> 4 files changed, 65 insertions(+), 38 deletions(-)
> 
> diff --git a/sql/item_sum.h b/sql/item_sum.h
> index e221d04397d..b10d9f5974d 100644
> --- a/sql/item_sum.h
> +++ b/sql/item_sum.h
> @@ -1879,7 +1879,8 @@ class Item_func_group_concat : public Item_sum
> bool warning_for_row;
> bool always_null;
> bool force_copy_fields;
> -  bool no_appended;
> +  /** True if result has been written to output buffer. */
> +  bool m_result_finalized;

Why does this variable use the m_ convention when others do not?
I think this is confusing (I stop for a moment to wonder how is it different
from all others every time I see the name). Please remove this.

What's "output buffer"? this->result ?
And what does "result" mean here - the entire return value of GROUP_CONCAT
function or some part of it?

Please improve the comment.

> /** Limits the rows in the result */
> Item *row_limit;
> /** Skips a particular number of rows in from the result*/

Ok to push after this is addressed.

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog