From: | Vik Fearing <vik(at)postgresfriends(dot)org> |
---|---|
To: | Georgios Kokolatos <gkokolatos(at)protonmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Cc: | Erik Rijkers <er(at)xs4all(dot)nl> |
Subject: | Re: GROUP BY DISTINCT |
Date: | 2021-03-02 16:51:52 |
Message-ID: | 4294e82f-a134-5dbb-d9d6-03d72dce2e1e@postgresfriends.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-docs pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
0001-implement-GROUP-BY-DISTINCT.v03.patch | text/x-patch | 19.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Georgios | 2021-03-02 19:21:42 | Re: GROUP BY DISTINCT |
Previous Message | Georgios Kokolatos | 2021-03-02 15:06:28 | Re: GROUP BY DISTINCT |
From | Date | Subject | |
---|---|---|---|
Next Message | Mark Dilger | 2021-03-02 16:57:52 | Re: [PATCH] Support empty ranges with bounds information |
Previous Message | Pantelis Theodosiou | 2021-03-02 16:51:01 | Re: [PATCH] Support empty ranges with bounds information |