Re: Final Patch for GROUPING SETS - unrecognized node type: 347

On 7.9.2014 15:11, Andrew Gierth wrote:
>>>>>> "Tomas" == Tomas Vondra <tv(at)fuzzy(dot)cz> writes:
>
> >> It's not one sort per grouping set, it's the minimal number of
> >> sorts needed to express the result as a union of ROLLUP
> >> clauses. The planner code will (I believe) always find the
> >> smallest number of sorts needed.
>
> Tomas> You're probably right. Although when doing GROUP BY CUBE
> Tomas> (a,b,c,a) I get one more ChainAggregate than with
> Tomas> CUBE(a,b,c). and we seem to compute all the aggregates
> Tomas> twice. Not sure if we need to address this though, because
> Tomas> it's mostly user's fault.
>
> Hm. Yeah, you're right that the number of sorts is not optimal
> there. We can look into that.

I don't think it's very critical, though. I was worried about it because
of the sorts, but if that gets tackled in patches following this
commitfest it seems OK.

> As for computing it all twice, there's currently no attempt to
> optimize multiple identical grouping sets into multiple projections
> of a single grouping set result. CUBE(a,b,c,a) has twice as many
> grouping sets as CUBE(a,b,c) does, even though all the extra ones are
> duplicates.

Shouldn't this be solved by eliminating the excessive ChainAggregate?
Although it probably changes GROUPING(...), so it's not just about
removing the duplicate column(s) from the CUBE.

Maybe preventing this completely (i.e. raising an ERROR with "duplicate
columns in CUBE/ROLLUP/... clauses") would be appropriate. Does the
standard says anything about this?

But arguably this is a minor issue, happening only when the user uses
the same column/expression twice. Hopefully the users don't do that too
often.

regards
Tomas