Re: Final Patch for GROUPING SETS

On 2015-05-12 20:40:49 +0200, Andres Freund wrote:
> On 2015-05-12 05:36:19 +0200, Andres Freund wrote:
> > What I dislike so far:
> > * Minor formatting things. Just going to fix and push the ones I
> >   dislike.
> > * The Hopcroft-Karp stuff not being separate
> > * The increased complexity of grouping_planner. It'd imo be good if some
> >   of that could be refactored into a separate function. Specifically the
> >   else if (parse->hasAggs || (parse->groupingSets && parse->groupClause))
> >   block.
> > * I think it'd not hurt to add rule deparse check for the function in
> >   GROUPING SETS case. I didn't see one at least.
> 
> * The code in nodeAgg.c isn't pretty in places. Stuff like if
>   (node->chain_depth > 0) estate->agg_chain_head = save_chain_head;...
>   Feels like a good bit of cleanup would be possible there.

In the executor I'd further like:
* to split agg_retrieve_direct into a version for grouping sets and one without. I think that'll be a pretty clear win
forclarity.
 
* to spin out common code between agg_retrieve_direct (in both the functions its split into), agg_retrieve_hashed and
agg_retrieve_chained.It should e.g. be fairly simple to spin out the tail end processing of a input group
(finalize_aggregateloop, ExecQual) into a separate function.
 

Andrew, are you going to be working on any of these?

Greetings,

Andres Freund