Subject: Re: GROUP BY DISTINCT
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Vik Fearing <vik@postgresfriends.org>,
 Georgios Kokolatos <gkokolatos@protonmail.com>,
 pgsql-hackers@lists.postgresql.org
Cc: Erik Rijkers <er@xs4all.nl>
References: <bf3805a8-d7d1-ae61-fece-761b7ff41ecc@postgresfriends.org>
 <c350ff6b-d70d-9952-be23-a18855b62589@postgresfriends.org>
 <161469758801.29967.4455067072614569957.pgcf@coridan.postgresql.org>
 <4294e82f-a134-5dbb-d9d6-03d72dce2e1e@postgresfriends.org>
 <b95ee556-c527-04b8-6cae-f55950dde94b@enterprisedb.com>
 <3817d5ec-f3e4-e4b9-d169-271ae5d3cbe4@postgresfriends.org>
 <8c44df08-bb64-3d78-da1a-d24b7c9db39d@enterprisedb.com>
Message-ID: <7137947a-87be-d04a-bf91-f42e47520daa@enterprisedb.com>
Date: Thu, 18 Mar 2021 18:25:47 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <8c44df08-bb64-3d78-da1a-d24b7c9db39d@enterprisedb.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/7137947a-87be-d04a-bf91-f42e47520daa%40enterprisedb.com>
Precedence: bulk

On 3/16/21 3:52 PM, Tomas Vondra wrote:
> 
> 
> On 3/16/21 9:21 AM, Vik Fearing wrote:
>> On 3/13/21 12:33 AM, Tomas Vondra wrote:
>>> Hi Vik,
>>>
>>> The patch seems quite ready, I have just two comments.
>>
>> Thanks for taking a look.
>>
>>> 1) Shouldn't this add another <indexterm> for DISTINCT, somewhere in the
>>> documentation? Now the index points just to the SELECT DISTINCT part.
>>
>> Good idea; I never think about the index.
>>
>>> 2) The part in gram.y that wraps/unwraps the boolean flag as an integer,
>>> in order to stash it in the group lists is rather ugly, IMHO. It forces
>>> all the places handling the list to be aware of this (there are not
>>> many, but still ...). And there are no other places doing (bool) intVal
>>> so it's not like there's a precedent for this.
>>
>> There is kind of a precedent for it, I was copying off of TriggerEvents
>> and func_alias_clause.
>>
> 
> I see. I was looking for "(bool) intVal" but you're right TriggerEvents
> code does something similar.
> 
>>> I think the clean solution is to make group_clause produce a struct with
>>> two fields, and just use that. Not sure how invasive that will be
>>> outside gram.y, though.
>>
>> I didn't want to create a whole new parse node for it, but Andrew Gierth
>> pointed me towards SelectLimit so I did it like that and I agree it is
>> much cleaner.
>>
> 
> I agree, that's much cleaner.
> 
>>> Also, the all_or_distinct vs. distinct_or_all seems a bit error-prone. I
>>> wonder if we can come up with some clearer names, describing the context
>>> of those types.
>>
>> I turned this into an enum for ALL/DISTINCT/default and the caller can
>> choose what it wants to do with default.  I think that's a lot cleaner,
>> too.  Maybe DISTINCT ON should be changed to fit in that?  I left it
>> alone for now.
>>
> 
> I think DISTINCT ON is a different kind of animal, because that is a
> list of expressions, not just a simple enum state.
> 
>> I also snuck in something that all of us overlooked which is outputting
>> the DISTINCT in ruleutils.c.  I didn't add a test for it but that would
>> have been an unfortunate bug.
>>
> 
> Oh!
> 
>> New patch attached, rebased on 15639d5e8f.
>>
> 
> Thanks. At this point it seems fine to me, no further comments.
> 

Pushed. Thanks for the patch.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company