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 for clarity.
* 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_aggregate loop,
ExecQual) into a separate function.

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

Greetings,

Andres Freund