Re: [HACKERS] GROUPING SETS revisited
Hello I found a break in GROUPING SETS implementation. Now I am playing with own executor and planner node and I can't to go forward :(. Probably this feature will need a significant update of our agg implementation. Probably needs a some similar structure like CTE but it can be a little bit reduced - there are a simple relation between source query and result query - I am not sure, if this has to be implemented via subqueries? The second question is relative big differencies between GROUP BY behave and GROUP BY GROUPING SETS behave. Now I don't know about way to join GROUP BY and GROUPING SETS together Any ideas welcome Regards Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GROUPING SETS revisited
Hello I was confused when I though so I found a solution of 1 shift/reduce conflict :( All identificators used for buildin functions have to be a col_name_keywords or reserved keyword. There is conflict with our (probably obsolete) feature SELECT colname(tabname). So for this moment the real solution is removing CUBE and ROLLUP from keywords and dynamically testing a funcname in transformation stage - what is slower and more ugly. ideas? Regards Pavel Stehule 2010/8/7 Pavel Stehule : > 2010/8/7 Joshua Tolley : >> On Thu, Aug 05, 2010 at 04:46:51PM +0200, Pavel Stehule wrote: >>> I am sending a updated version. >> >> I've been looking at the changes to gram.y, and noted the comment under >> func_expr >> where you added CUBE and ROLLUP definitions. It says that CUBE can't be a >> reserved keyword because it's already used in the cube contrib module. But >> then the changes to kwlist.h include this: >> > > I am little bit confused now - it's bad comment - and I have to verify > it. What I remember, we cannot to use a two parser's rules, because it > going to a conflict. So there have to be used a trick with a moving to > decision to transform stage, where we have a context info. I have to > recheck a minimal level - probably it can't be a RESERVED_KEYWORD. > Because then we can't to create a function "cube". > >> + PG_KEYWORD("cube", CUBE, RESERVED_KEYWORD) >> ... >> + PG_KEYWORD("rollup", ROLLUP, RESERVED_KEYWORD) >> >> ...and CUBE and ROLLUP are added in gram.y under the reserved_keyword list. I >> realize things like CURRENT_TIME, that also have special entries in the >> func_expr grammar, are also reserved keywords, but this all seems at odds >> with >> the comment. What am I missing? Is the comment simply pointing out that the >> designation of CUBE and ROLLUP as reserved keywords will have to change at >> some point, but it hasn't been implemented yet (or no one has figured out how >> to do it)? >> >> -- >> Joshua Tolley / eggyknap >> End Point Corporation >> http://www.endpoint.com >> >> -BEGIN PGP SIGNATURE- >> Version: GnuPG v1.4.9 (GNU/Linux) >> >> iEYEARECAAYFAkxcjSIACgkQRiRfCGf1UMPpCwCcCHBh/1NiLykIcVYgPyfbIegF >> xq0AoID75rCPiW8yf29OSkaJVza1FQt5 >> =PcLs >> -END PGP SIGNATURE- >> >> > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GROUPING SETS revisited
2010/8/7 Joshua Tolley : > On Thu, Aug 05, 2010 at 04:46:51PM +0200, Pavel Stehule wrote: >> I am sending a updated version. > > I've been looking at the changes to gram.y, and noted the comment under > func_expr > where you added CUBE and ROLLUP definitions. It says that CUBE can't be a > reserved keyword because it's already used in the cube contrib module. But > then the changes to kwlist.h include this: > I am little bit confused now - it's bad comment - and I have to verify it. What I remember, we cannot to use a two parser's rules, because it going to a conflict. So there have to be used a trick with a moving to decision to transform stage, where we have a context info. I have to recheck a minimal level - probably it can't be a RESERVED_KEYWORD. Because then we can't to create a function "cube". > + PG_KEYWORD("cube", CUBE, RESERVED_KEYWORD) > ... > + PG_KEYWORD("rollup", ROLLUP, RESERVED_KEYWORD) > > ...and CUBE and ROLLUP are added in gram.y under the reserved_keyword list. I > realize things like CURRENT_TIME, that also have special entries in the > func_expr grammar, are also reserved keywords, but this all seems at odds with > the comment. What am I missing? Is the comment simply pointing out that the > designation of CUBE and ROLLUP as reserved keywords will have to change at > some point, but it hasn't been implemented yet (or no one has figured out how > to do it)? > > -- > Joshua Tolley / eggyknap > End Point Corporation > http://www.endpoint.com > > -BEGIN PGP SIGNATURE- > Version: GnuPG v1.4.9 (GNU/Linux) > > iEYEARECAAYFAkxcjSIACgkQRiRfCGf1UMPpCwCcCHBh/1NiLykIcVYgPyfbIegF > xq0AoID75rCPiW8yf29OSkaJVza1FQt5 > =PcLs > -END PGP SIGNATURE- > > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GROUPING SETS revisited
On Thu, Aug 05, 2010 at 04:46:51PM +0200, Pavel Stehule wrote: > I am sending a updated version. I've been looking at the changes to gram.y, and noted the comment under func_expr where you added CUBE and ROLLUP definitions. It says that CUBE can't be a reserved keyword because it's already used in the cube contrib module. But then the changes to kwlist.h include this: + PG_KEYWORD("cube", CUBE, RESERVED_KEYWORD) ... + PG_KEYWORD("rollup", ROLLUP, RESERVED_KEYWORD) ...and CUBE and ROLLUP are added in gram.y under the reserved_keyword list. I realize things like CURRENT_TIME, that also have special entries in the func_expr grammar, are also reserved keywords, but this all seems at odds with the comment. What am I missing? Is the comment simply pointing out that the designation of CUBE and ROLLUP as reserved keywords will have to change at some point, but it hasn't been implemented yet (or no one has figured out how to do it)? -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com signature.asc Description: Digital signature
Re: [HACKERS] GROUPING SETS revisited
I found other issue :( postgres=# select name, place from cars group by grouping sets(name, place,()); name | place ---+ bmw | skoda | opel | | germany | czech rep. skoda | czech rep. skoda | germany bmw | czech rep. bmw | germany opel | czech rep. opel | germany (11 rows) postgres=# explain select name, place from cars group by grouping sets(name, place,()); QUERY PLAN -- Append (cost=36.98..88.55 rows=1230 width=54) CTE GroupingSets -> Seq Scan on cars (cost=0.00..18.30 rows=830 width=68) -> HashAggregate (cost=18.68..20.68 rows=200 width=32) -> CTE Scan on "GroupingSets" (cost=0.00..16.60 rows=830 width=32) -> HashAggregate (cost=18.68..20.68 rows=200 width=32) -> CTE Scan on "GroupingSets" (cost=0.00..16.60 rows=830 width=32) -> CTE Scan on "GroupingSets" (cost=0.00..16.60 rows=830 width=64) (8 rows) the combination of nonagregates and empty sets do a problems - because we can't ensure agg mode without aggregates or group by. But it is only minor issue 2010/8/5 Joshua Tolley : > On Thu, Aug 05, 2010 at 04:46:51PM +0200, Pavel Stehule wrote: >> So Joshua, can you look on code? > > Sure... thanks :) > > -- > Joshua Tolley / eggyknap > End Point Corporation > http://www.endpoint.com > > -BEGIN PGP SIGNATURE- > Version: GnuPG v1.4.9 (GNU/Linux) > > iEYEARECAAYFAkxa1NsACgkQRiRfCGf1UMPwzQCgjz52P86Yx4ac4aRkKwjn8OHK > 6/EAoJ/CjXEyPaLpx39SI5bKQPz+AwBR > =Mi2J > -END PGP SIGNATURE- > > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GROUPING SETS revisited
On Thu, Aug 05, 2010 at 04:46:51PM +0200, Pavel Stehule wrote: > So Joshua, can you look on code? Sure... thanks :) -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com signature.asc Description: Digital signature
Re: [HACKERS] GROUPING SETS revisited
On Thu, Aug 05, 2010 at 06:21:18AM +0200, Pavel Stehule wrote: > I hope, so next week you can do own work on this job - I am not a > native speaker, and my code will need a checking and fixing comments I haven't entirely figured out how the code in the old patch works, but I promise I *can* edit comments/docs :) -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com signature.asc Description: Digital signature
Re: [HACKERS] GROUPING SETS revisited
2010/8/4 Joshua Tolley : > On Wed, Aug 04, 2010 at 04:44:05AM +0200, Pavel Stehule wrote: >> > Yeah, I seem to have done a poor job of producing the patch based on the >> > repository I was working from. That said, it seems Pavel's working >> > actively on >> > a patch anyway, so perhaps my updating the old one isn't all that >> > worthwhile. >> > Pavel, is your code somewhere that we can get to it? >> > >> >> not now. please wait a week. > > That works for me. I'm glad to try doing a better job of putting together my > version of the patch, if anyone thinks it's useful, but it seems that since > Pavel's code is due to appear sometime in the foreseeable future, there's not > much point in my doing that. > I hope, so next week you can do own work on this job - I am not a native speaker, and my code will need a checking and fixing comments Regards Pavel > -- > Joshua Tolley / eggyknap > End Point Corporation > http://www.endpoint.com > > -BEGIN PGP SIGNATURE- > Version: GnuPG v1.4.9 (GNU/Linux) > > iEYEARECAAYFAkxZkp8ACgkQRiRfCGf1UMMUcwCfcPayQbWRUYwhpCF1f24LsdD9 > H/gAnRzCEq6yLX/RVLLi88ROhurOzbhK > =gUPx > -END PGP SIGNATURE- > > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GROUPING SETS revisited
On Wed, Aug 04, 2010 at 04:44:05AM +0200, Pavel Stehule wrote: > > Yeah, I seem to have done a poor job of producing the patch based on the > > repository I was working from. That said, it seems Pavel's working actively > > on > > a patch anyway, so perhaps my updating the old one isn't all that > > worthwhile. > > Pavel, is your code somewhere that we can get to it? > > > > not now. please wait a week. That works for me. I'm glad to try doing a better job of putting together my version of the patch, if anyone thinks it's useful, but it seems that since Pavel's code is due to appear sometime in the foreseeable future, there's not much point in my doing that. -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com signature.asc Description: Digital signature
Re: [HACKERS] GROUPING SETS revisited
2010/8/3 Joshua Tolley : > On Tue, Aug 03, 2010 at 12:58:03PM -0700, David Fetter wrote: >> On Mon, Aug 02, 2010 at 11:50:00PM -0600, Josh Tolley wrote: >> > In case anyone's interested, I've taken the CTE-based grouping sets >> > patch from [1] and made it apply to 9.1, attached. I haven't yet >> > done things like checked it for whitespace consistency, style >> > conformity, or anything else, but (tuits permitting) hope to figure >> > out how it works and get it closer to commitability in some upcoming >> > commitfest. >> > >> > I mention it here so that if someone else is working on it, we can >> > avoid duplicated effort, and to see if a CTE-based grouping sets >> > implementation is really the way we think we want to go. >> > >> > [1] >> > http://archives.postgresql.org/pgsql-hackers/2009-05/msg00700.php >> >> I've added back one file in the patch enclosed here. I'm still >> getting compile fails from >> >> CC="ccache gcc" ./configure --prefix=$PG_PREFIX >> --with-pgport=$PGPORT --with-perl --with-libxml --enable-debug >> --enable-cassert >> make >> >> Log from that also enclosed. >> > > Yeah, I seem to have done a poor job of producing the patch based on the > repository I was working from. That said, it seems Pavel's working actively on > a patch anyway, so perhaps my updating the old one isn't all that worthwhile. > Pavel, is your code somewhere that we can get to it? > not now. please wait a week. Regards Pavel > -- > Joshua Tolley / eggyknap > End Point Corporation > http://www.endpoint.com > > -BEGIN PGP SIGNATURE- > Version: GnuPG v1.4.9 (GNU/Linux) > > iEYEARECAAYFAkxYeiQACgkQRiRfCGf1UMPlEQCff+I4sCGtR+lzUs6Wb5JKi7Uu > 3qYAnjLHzHzyMSHHX55QsphkaBbEJ0Zf > =uRqV > -END PGP SIGNATURE- > > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GROUPING SETS revisited
On Tue, Aug 03, 2010 at 12:58:03PM -0700, David Fetter wrote: > On Mon, Aug 02, 2010 at 11:50:00PM -0600, Josh Tolley wrote: > > In case anyone's interested, I've taken the CTE-based grouping sets > > patch from [1] and made it apply to 9.1, attached. I haven't yet > > done things like checked it for whitespace consistency, style > > conformity, or anything else, but (tuits permitting) hope to figure > > out how it works and get it closer to commitability in some upcoming > > commitfest. > > > > I mention it here so that if someone else is working on it, we can > > avoid duplicated effort, and to see if a CTE-based grouping sets > > implementation is really the way we think we want to go. > > > > [1] > > http://archives.postgresql.org/pgsql-hackers/2009-05/msg00700.php > > I've added back one file in the patch enclosed here. I'm still > getting compile fails from > > CC="ccache gcc" ./configure --prefix=$PG_PREFIX --with-pgport=$PGPORT > --with-perl --with-libxml --enable-debug --enable-cassert > make > > Log from that also enclosed. > Yeah, I seem to have done a poor job of producing the patch based on the repository I was working from. That said, it seems Pavel's working actively on a patch anyway, so perhaps my updating the old one isn't all that worthwhile. Pavel, is your code somewhere that we can get to it? -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com signature.asc Description: Digital signature
Re: [HACKERS] GROUPING SETS revisited
On Mon, Aug 02, 2010 at 11:50:00PM -0600, Josh Tolley wrote: > In case anyone's interested, I've taken the CTE-based grouping sets > patch from [1] and made it apply to 9.1, attached. I haven't yet > done things like checked it for whitespace consistency, style > conformity, or anything else, but (tuits permitting) hope to figure > out how it works and get it closer to commitability in some upcoming > commitfest. > > I mention it here so that if someone else is working on it, we can > avoid duplicated effort, and to see if a CTE-based grouping sets > implementation is really the way we think we want to go. > > [1] > http://archives.postgresql.org/pgsql-hackers/2009-05/msg00700.php I've added back one file in the patch enclosed here. I'm still getting compile fails from CC="ccache gcc" ./configure --prefix=$PG_PREFIX --with-pgport=$PGPORT --with-perl --with-libxml --enable-debug --enable-cassert make Log from that also enclosed. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate diff --git a/src/backend/parser/Makefile b/src/backend/parser/Makefile index a8f4c07..fb248a6 100644 --- a/src/backend/parser/Makefile +++ b/src/backend/parser/Makefile @@ -15,7 +15,7 @@ override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS) OBJS= analyze.o gram.o keywords.o kwlookup.o parser.o \ parse_agg.o parse_clause.o parse_coerce.o parse_cte.o parse_expr.o \ parse_func.o parse_node.o parse_oper.o parse_param.o parse_relation.o \ - parse_target.o parse_type.o parse_utilcmd.o scansup.o + parse_target.o parse_type.o parse_utilcmd.o scansup.o parse_gsets.o FLEXFLAGS = -CF diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 6b99a10..1b579a8 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -34,6 +34,7 @@ #include "parser/parse_clause.h" #include "parser/parse_coerce.h" #include "parser/parse_cte.h" +#include "parser/parse_gsets.h" #include "parser/parse_oper.h" #include "parser/parse_param.h" #include "parser/parse_relation.h" @@ -150,6 +151,163 @@ parse_sub_analyze(Node *parseTree, ParseState *parentParseState, } /* + * process GROUPING SETS + */ +static SelectStmt * +makeSelectStmt(List *targetList, List *fromClause) +{ + SelectStmt *n = makeNode(SelectStmt); + n->distinctClause = NULL; + n->intoClause = NULL; + n->targetList = targetList; + n->fromClause = fromClause; + n->whereClause = NULL; + n->groupClause = NULL; + n->havingClause = NULL; + n->windowClause = NIL; + n->withClause = NULL; + n->valuesLists = NIL; + n->sortClause = NIL; + n->limitOffset = NULL; + n->limitCount = NULL; + n->lockingClause = NIL; + n->op = SETOP_NONE; + n->all = false; + n->larg = NULL; + n->rarg = NULL; + return n; +} + +static List * +makeStarTargetList(void) +{ + ResTarget *rt = makeNode(ResTarget); + + rt->name = NULL; + rt->indirection = NIL; + rt->val = (Node *) makeNode(ColumnRef); + ((ColumnRef *) rt->val)->fields = list_make1(makeNode(A_Star)); + rt->location = -1; + + return list_make1(rt); +} + +static SelectStmt * +transformGroupingSets(ParseState *pstate, SelectStmt *stmt) +{ + if (stmt->groupClause && IsA(stmt->groupClause, GroupByClause)) + { + GroupingSetsSpec *gss = (GroupingSetsSpec *) expandGroupingSets(pstate, + (List *)((GroupByClause *)stmt->groupClause)->fields); + + if (pstate->p_hasGroupingSets) + { + CommonTableExpr *cte = makeNode(CommonTableExpr); + SelectStmt *cteedstmt; + int ngroupingsets = list_length(gss->set_list) + (gss->has_empty_set ? 1 : 0); + boolall = ((GroupByClause *) stmt->groupClause)->all; + + cteedstmt = makeSelectStmt(NIL, NIL); + cteedstmt->intoClause = stmt->intoClause; + cteedstmt->sortClause = stmt->sortClause; + cteedstmt->limitOffset = stmt->limitOffset; + cteedstmt->limitCount = stmt->limitCount; + cteedstmt->lockingClause = stmt->lockingClause; + + cte->ctename = "**g**"; + cte->ctequery = (Node *) stmt; + cte->location = -1; + + cteedstmt->withClause = makeNode(WithClause); + cteedstmt->withClause->ctes = list_make1(cte); + cteedstmt->w
Re: [HACKERS] GROUPING SETS revisited
2010/8/3 Hitoshi Harada : > 2010/8/3 Pavel Stehule : >> Hello >> >> 2010/8/3 Joshua Tolley : >>> In case anyone's interested, I've taken the CTE-based grouping sets patch >>> from >>> [1] and made it apply to 9.1, attached. I haven't yet done things like >>> checked >>> it for whitespace consistency, style conformity, or anything else, but >>> (tuits >>> permitting) hope to figure out how it works and get it closer to >>> commitability >>> in some upcoming commitfest. >>> >>> I mention it here so that if someone else is working on it, we can avoid >>> duplicated effort, and to see if a CTE-based grouping sets implementation is >>> really the way we think we want to go. >>> >> >> I am plaing with it now :). I have a plan to replace CTE with similar >> but explicit executor node. The main issue of existing patch was using >> just transformation to CTE. I agree, so it isn't too much extensiable >> in future. Now I am cleaning identifiers little bit. Any colaboration >> is welcome. >> >> My plan: >> 1) clean CTE patch >> 2) replace CTE with explicit executor node, but still based on tuplestore >> 3) when will be possible parallel processing based on hash agg - then >> we don't need to use tuplestore > > Couldn't you explain what exactly "explicit executor node"? I hope we > can share your image to develop it further than only transformation to > CTE. I have a one reason Implementation based on CTE doesn't create space for possible optimalisations (I think now, maybe it isn't true). It is good for initial or referencial implementation - but it can be too complex, when we will try to append some optimalizations - like parallel hash agg processing, direct data reading without tuplestore. If you are, as CTE author, thinking so these features are possible in non recursive CTE too, I am not agains. I hope so this week I'll have a CTE based patch - and we can talk about next direction. I see as possible performance issue using a tuplestore - there are lot of cases where repeating of source query can be faster. If I remember well, Tom has a objection, so transformation to CTE is too early - in parser. So It will be first change. Executor node can be CTE. regards Pavel p.s. I am sure, so there are lot of task, that can be solved together with non recursive CTE. > > > Regards, > > -- > Hitoshi Harada > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GROUPING SETS revisited
2010/8/3 Pavel Stehule : > Hello > > 2010/8/3 Joshua Tolley : >> In case anyone's interested, I've taken the CTE-based grouping sets patch >> from >> [1] and made it apply to 9.1, attached. I haven't yet done things like >> checked >> it for whitespace consistency, style conformity, or anything else, but (tuits >> permitting) hope to figure out how it works and get it closer to >> commitability >> in some upcoming commitfest. >> >> I mention it here so that if someone else is working on it, we can avoid >> duplicated effort, and to see if a CTE-based grouping sets implementation is >> really the way we think we want to go. >> > > I am plaing with it now :). I have a plan to replace CTE with similar > but explicit executor node. The main issue of existing patch was using > just transformation to CTE. I agree, so it isn't too much extensiable > in future. Now I am cleaning identifiers little bit. Any colaboration > is welcome. > > My plan: > 1) clean CTE patch > 2) replace CTE with explicit executor node, but still based on tuplestore > 3) when will be possible parallel processing based on hash agg - then > we don't need to use tuplestore Couldn't you explain what exactly "explicit executor node"? I hope we can share your image to develop it further than only transformation to CTE. Regards, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GROUPING SETS revisited
Hello 2010/8/3 Joshua Tolley : > In case anyone's interested, I've taken the CTE-based grouping sets patch from > [1] and made it apply to 9.1, attached. I haven't yet done things like checked > it for whitespace consistency, style conformity, or anything else, but (tuits > permitting) hope to figure out how it works and get it closer to commitability > in some upcoming commitfest. > > I mention it here so that if someone else is working on it, we can avoid > duplicated effort, and to see if a CTE-based grouping sets implementation is > really the way we think we want to go. > I am plaing with it now :). I have a plan to replace CTE with similar but explicit executor node. The main issue of existing patch was using just transformation to CTE. I agree, so it isn't too much extensiable in future. Now I am cleaning identifiers little bit. Any colaboration is welcome. My plan: 1) clean CTE patch 2) replace CTE with explicit executor node, but still based on tuplestore 3) when will be possible parallel processing based on hash agg - then we don't need to use tuplestore comments?? Regards Pavel > [1] http://archives.postgresql.org/pgsql-hackers/2009-05/msg00700.php > > -- > Joshua Tolley / eggyknap > End Point Corporation > http://www.endpoint.com > > -BEGIN PGP SIGNATURE- > Version: GnuPG v1.4.9 (GNU/Linux) > > iEYEARECAAYFAkxXrggACgkQRiRfCGf1UMMlCQCglaIdtPj8Qe6G60V2LHn5pFNn > kgIAniXRgIQEbVrK/eDVZnmKCzw33lT9 > =XVVV > -END PGP SIGNATURE- > > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] GROUPING SETS revisited
In case anyone's interested, I've taken the CTE-based grouping sets patch from [1] and made it apply to 9.1, attached. I haven't yet done things like checked it for whitespace consistency, style conformity, or anything else, but (tuits permitting) hope to figure out how it works and get it closer to commitability in some upcoming commitfest. I mention it here so that if someone else is working on it, we can avoid duplicated effort, and to see if a CTE-based grouping sets implementation is really the way we think we want to go. [1] http://archives.postgresql.org/pgsql-hackers/2009-05/msg00700.php -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com diff --git a/src/backend/parser/Makefile b/src/backend/parser/Makefile index a8f4c07..fb248a6 100644 *** a/src/backend/parser/Makefile --- b/src/backend/parser/Makefile *** override CPPFLAGS := -I. -I$(srcdir) $(C *** 15,21 OBJS= analyze.o gram.o keywords.o kwlookup.o parser.o \ parse_agg.o parse_clause.o parse_coerce.o parse_cte.o parse_expr.o \ parse_func.o parse_node.o parse_oper.o parse_param.o parse_relation.o \ ! parse_target.o parse_type.o parse_utilcmd.o scansup.o FLEXFLAGS = -CF --- 15,21 OBJS= analyze.o gram.o keywords.o kwlookup.o parser.o \ parse_agg.o parse_clause.o parse_coerce.o parse_cte.o parse_expr.o \ parse_func.o parse_node.o parse_oper.o parse_param.o parse_relation.o \ ! parse_target.o parse_type.o parse_utilcmd.o scansup.o parse_gsets.o FLEXFLAGS = -CF diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 6b99a10..1b579a8 100644 *** a/src/backend/parser/analyze.c --- b/src/backend/parser/analyze.c *** *** 34,39 --- 34,40 #include "parser/parse_clause.h" #include "parser/parse_coerce.h" #include "parser/parse_cte.h" + #include "parser/parse_gsets.h" #include "parser/parse_oper.h" #include "parser/parse_param.h" #include "parser/parse_relation.h" *** parse_sub_analyze(Node *parseTree, Parse *** 150,155 --- 151,313 } /* + * process GROUPING SETS + */ + static SelectStmt * + makeSelectStmt(List *targetList, List *fromClause) + { + SelectStmt *n = makeNode(SelectStmt); + n->distinctClause = NULL; + n->intoClause = NULL; + n->targetList = targetList; + n->fromClause = fromClause; + n->whereClause = NULL; + n->groupClause = NULL; + n->havingClause = NULL; + n->windowClause = NIL; + n->withClause = NULL; + n->valuesLists = NIL; + n->sortClause = NIL; + n->limitOffset = NULL; + n->limitCount = NULL; + n->lockingClause = NIL; + n->op = SETOP_NONE; + n->all = false; + n->larg = NULL; + n->rarg = NULL; + return n; + } + + static List * + makeStarTargetList(void) + { + ResTarget *rt = makeNode(ResTarget); + + rt->name = NULL; + rt->indirection = NIL; + rt->val = (Node *) makeNode(ColumnRef); + ((ColumnRef *) rt->val)->fields = list_make1(makeNode(A_Star)); + rt->location = -1; + + return list_make1(rt); + } + + static SelectStmt * + transformGroupingSets(ParseState *pstate, SelectStmt *stmt) + { + if (stmt->groupClause && IsA(stmt->groupClause, GroupByClause)) + { + GroupingSetsSpec *gss = (GroupingSetsSpec *) expandGroupingSets(pstate, + (List *)((GroupByClause *)stmt->groupClause)->fields); + + if (pstate->p_hasGroupingSets) + { + CommonTableExpr *cte = makeNode(CommonTableExpr); + SelectStmt *cteedstmt; + int ngroupingsets = list_length(gss->set_list) + (gss->has_empty_set ? 1 : 0); + bool all = ((GroupByClause *) stmt->groupClause)->all; + + cteedstmt = makeSelectStmt(NIL, NIL); + cteedstmt->intoClause = stmt->intoClause; + cteedstmt->sortClause = stmt->sortClause; + cteedstmt->limitOffset = stmt->limitOffset; + cteedstmt->limitCount = stmt->limitCount; + cteedstmt->lockingClause = stmt->lockingClause; + + cte->ctename = "**g**"; + cte->ctequery = (Node *) stmt; + cte->location = -1; + + cteedstmt->withClause = makeNode(WithClause); + cteedstmt->withClause->ctes = list_make1(cte); + cteedstmt->withClause->recursive = false; + cteedstmt->withClause->location = -1; + + /* when is more than one grouping set, then we should generate setop node */ + if (ngroupingsets > 1) + { + /* add quuery under union all for every grouping set */ + SelectStmt *larg = NULL; + SelectStmt *rarg; + ListCell*lc; + + foreach(lc, gss->set_list) + { + List *groupClause; + + Assert(IsA(lfirst(lc), List)); + groupClause = (List *) lfirst(lc); + + if (larg == NULL) + { + larg = makeSelectStmt(copyObject(stmt->targetList), + list_make1(makeRangeVar(NULL, "**g**", -1))); + larg->groupClause = (Node *) groupClause; + larg->havingClause = copyObject(stmt->havingClause); + } + else + { + SelectStmt *setop = makeSelectStmt(NIL, NIL); + + rarg