Re: GROUP BY DISTINCT

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