Subject: Re: GROUP BY DISTINCT
To: Georgios Kokolatos <gkokolatos@protonmail.com>,
 pgsql-hackers@lists.postgresql.org
References: <bf3805a8-d7d1-ae61-fece-761b7ff41ecc@postgresfriends.org>
 <c350ff6b-d70d-9952-be23-a18855b62589@postgresfriends.org>
 <161469758801.29967.4455067072614569957.pgcf@coridan.postgresql.org>
Cc: Erik Rijkers <er@xs4all.nl>
From: Vik Fearing <vik@postgresfriends.org>
Message-ID: <4294e82f-a134-5dbb-d9d6-03d72dce2e1e@postgresfriends.org>
Date: Tue, 2 Mar 2021 17:51:52 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: 
 <161469758801.29967.4455067072614569957.pgcf@coridan.postgresql.org>
Content-Type: multipart/mixed;
 boundary="------------FA0E50240E4CFE465812C909"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------FA0E50240E4CFE465812C909
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

On 3/2/21 4:06 PM, Georgios Kokolatos wrote:
> As a minor gripe, I would note the addition of list_int_cmp.
> The block
> 
> +               /* Sort each groupset individually */
> +               foreach(cell, result)
> +                       list_sort(lfirst(cell), list_int_cmp);
> 
> Can follow suit from the rest of the code, and define a static cmp_list_int_asc(), as
> indeed the same patch does for cmp_list_len_contents_asc.
> This is indeed point of which I will not hold a too strong opinion.

I did it this way because list_int_cmp is a general purpose function for
int lists that can be reused elsewhere in the future.  Whereas
cmp_list_len_contents_asc is very specific to this case so I kept it local.

I'm happy to change it around if that's what consensus wants.

> Overall :+1: from me.

Thanks for looking at it!

> I will be bumping to 'Ready for Committer' unless objections.

In that case, here is another patch that fixes a typo in the docs
mentioned privately to me by Erik.  The typo (and a gratuitous rebase)
is the only change to what you just reviewed.
-- 
Vik Fearing

--------------FA0E50240E4CFE465812C909
Content-Type: text/x-patch; charset=UTF-8;
 name="0001-implement-GROUP-BY-DISTINCT.v03.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001-implement-GROUP-BY-DISTINCT.v03.patch"