Thread: Grouping Sets: Fix unrecognized node type bug

>>>>> "Jeevan" == Jeevan Chalke <jeevan.chalke@enterprisedb.com> writes:
Jeevan> Hi,Jeevan> It looks like we do support nested GROUPING SETS, I mean SetsJeevan> withing Sets, not other types.
Howeverthis nesting is broken.
 

Good catch, but I'm not yet sure your fix is correct; I'll need to look
into that.

-- 
Andrew (irc:RhodiumToad)

Hello, this looks to be a kind of thinko. The attached patch
fixes it.

===
According to the comment of transformGroupingSet, it assumes that
the given GROUPING SETS node is already flatted out and
flatten_grouping_sets() does that. The details of the
transformation is described in the comment for the function.

The problmen is what does the function for nested grouping sets.

> Node       *n2 = flatten_grouping_sets(lfirst(l2), false, NULL);
> result_set = lappend(result_set, n2);

This does not flattens the list as required. n2 should be
concatenated if it is a list. The attached small patch fixes it
and the problematic query returns sane (perhaps) result.

# Though I don't know the exact definition of the syntax..

=# select sum(c) from gstest2 group by grouping sets ((), grouping sets ((), grouping sets (())));sum 
----- 12 12 12
(3 rows)

=# select sum(c) from gstest2 group by grouping sets ((a), grouping sets ((b), grouping sets ((c))));sum 
----- 10  2  6  6  8  4
(6 rows)

regards,

At Fri, 17 Jul 2015 11:37:26 +0530, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote in
<CAM2+6=XPRgumbqWTsczbECC3xjv4zH1ryQ3fwds5uajON1iq2A@mail.gmail.com>
> >  Jeevan> It looks like we do support nested GROUPING SETS, I mean Sets
> >  Jeevan> withing Sets, not other types.  However this nesting is broken.
> >
> > Good catch, but I'm not yet sure your fix is correct; I'll need to look
> > into that.
> >
> 
> Sure. Thanks.
> 
> However I wonder why we are supporting GROUPING SETS inside GROUPING SETS.
> On Oracle, it is throwing an error.
> We are not trying to be Oracle compatible, but just curious to know.
> 
> I have tried restricting it in attached patch.
> 
> But it may require few comment adjustment.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index e90e1d6..708ebc9 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -1804,8 +1804,10 @@ flatten_grouping_sets(Node *expr, bool toplevel, bool *hasGroupingSets)
foreach(l2,gset->content)                {                    Node       *n2 = flatten_grouping_sets(lfirst(l2), false,
NULL);
-
-                    result_set = lappend(result_set, n2);
+                    if (IsA(n2, List))
+                        result_set = list_concat(result_set, (List *)n2);
+                    else
+                        result_set = lappend(result_set, n2);                }                /*

>>>>> "Kyotaro" == Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.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.

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).

--
Andrew (irc:RhodiumToad)

Hello,

At Mon, 20 Jul 2015 15:45:21 +0530, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote in
<CAM2+6=X9QWgbjJrR-dcLXh-RvvpGy=9ENhUOGHzrxhcJ2kVDSQ@mail.gmail.com>
> On Sat, Jul 18, 2015 at 12:27 AM, Andrew Gierth <andrew@tao11.riddles.org.uk
> > wrote:
> 
> > >>>>> "Kyotaro" == Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.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

On 2015-07-17 11:37:26 +0530, Jeevan Chalke wrote:
> However I wonder why we are supporting GROUPING SETS inside GROUPING SETS.
> On Oracle, it is throwing an error.
> We are not trying to be Oracle compatible, but just curious to know.

The SQL specification seems to be pretty unambigous about supporting
nested grouping set specifications. Check 7.9 <group by
clause>: <grouping set> (nested inside a <grouping sets specification>)
can contain <grouping sets specification>.

Regards,

Andres

On 2015-07-17 19:57:22 +0100, Andrew Gierth wrote:
> Attached is the current version of my fix (with Jeevan's regression
> tests plus one of mine).

Pushed, thanks for the report and fix!