From: Vik Fearing <vik@postgresfriends.org>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Georgios Kokolatos <gkokolatos@protonmail.com>
To: pgsql-hackers@lists.postgresql.org
Cc: Erik Rijkers <er@xs4all.nl>
Subject: Re: GROUP BY DISTINCT
Date: Tue, 16 Mar 2021 09:21:03 +0100
Message-ID: <3817d5ec-f3e4-e4b9-d169-271ae5d3cbe4@postgresfriends.org> (raw)
In-Reply-To: <b95ee556-c527-04b8-6cae-f55950dde94b@enterprisedb.com>
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>
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 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.
> 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 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.
New patch attached, rebased on 15639d5e8f.
--
Vik Fearing
Attachments:
[text/x-patch] 0001-implement-GROUP-BY-DISTINCT.v04.patch (0B, ../3817d5ec-f3e4-e4b9-d169-271ae5d3cbe4@postgresfriends.org/2-0001-implement-GROUP-BY-DISTINCT.v04.patch)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-docs@postgresql.org
Cc: vik@postgresfriends.org, tomas.vondra@enterprisedb.com, gkokolatos@protonmail.com, pgsql-hackers@lists.postgresql.org, er@xs4all.nl
Subject: Re: GROUP BY DISTINCT
In-Reply-To: <3817d5ec-f3e4-e4b9-d169-271ae5d3cbe4@postgresfriends.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox