On Tue, Oct 21, 2025 at 1:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > I like the concept here, but not so much the details. Pulling
> > expand_grouping_sets out of preprocess_grouping_sets feels weird.
> > I guess it's all right given that preprocess_grouping_sets doesn't
> > manipulate the parse tree otherwise, but you didn't update the comment
> > for preprocess_grouping_sets. Also it might be a good idea to have a
> > test case demonstrating why v2 wasn't good enough.
> Here's a v4 with some concrete suggestions for comment changes,
> plus the extra test case. (I did some tiny cosmetic fooling with
> preprocess_grouping_sets too, because the order of its initial
> operations seemed quite weird to me.)
Yeah, I noticed the out-of-date comment for preprocess_grouping_sets
shortly after sending out the v3 patch, but I was too tired last night
to update and resend it. Thanks for the suggestions in v4 -- all the
changes look good to me.
Regarding the tests, I think we could add another test query to cover
the case with no empty grouping sets and degenerate HAVING clauses.
This way, all cases for the HAVING pushdown optimization with grouping
sets should be covered.
I've added such a test in v5, along with a commit message. Nothing
else has changed. I'll push this patch soon, barring any objections.
- Richard
