Re: [HACKERS] Grouping Sets: Fix unrecognized node type bug

Hello,

At Mon, 20 Jul 2015 15:45:21 +0530, Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com> wrote in <CAM2+6=X9QWgbjJrR-dcLXh-RvvpGy=9ENhUOGHzrxhcJ2kVDSQ(at)mail(dot)gmail(dot)com>
> On Sat, Jul 18, 2015 at 12:27 AM, Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk
> > wrote:
>
> > >>>>> "Kyotaro" == Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > writes:
> >
> > Kyotaro> Hello, this looks to be a kind of thinko. The attached patch
> > Kyotaro> fixes it.
> >
> > No, that's still wrong. Just knowing that there is a List is not enough
> > to tell whether to concat it or append it.

Thank you. I've missed the non-grouping-set cases.

> > Jeevan's original patch tries to get around this by making the RowExpr
> > case wrap another List around its result (which is then removed by the
> > concat), but this is the wrong approach too because it breaks nested
> > RowExprs (which isn't valid syntax in the spec, because the spec allows
> > only column references in GROUP BY, not arbitrary expressions, but which
> > we have no reason not to support).
>
> > Attached is the current version of my fix (with Jeevan's regression
> > tests plus one of mine).
> >
>
> Looks good to me.

It also looks for me to work as expected and to be in good shape.

The two foreach loops for T_GroupingSet and T_List became to look
very simiar but they don't seem can be merged in reasonable
shape.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center