Skip site navigation (1) Skip section navigation (2)

[Review] Grouping Sets patch

From: "Ibrar Ahmed" <ibrar(dot)ahmad(at)gmail(dot)com>
To: pavel(dot)stehule(at)gmail(dot)com
Cc: Postgres <pgsql-hackers(at)postgresql(dot)org>
Subject: [Review] Grouping Sets patch
Date: 2008-11-24 10:59:49
Message-ID: 8494ccf60811240259s4a5bc09aj903a625289444d17@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
Hi Stehule,

I have looked at the patch and it looks great. Here are my observation

Compilation
----------------
1 - Patch applied successfully on CVS-HEAD
2 - No compilation error found

Code
-------
1 - Style of code is very close to the existing PG code.
2 - Comments look OK to me.
3 - I haven't looked deep into the code. As this is a WIP patch so I
gave my emphasis on testing and verifying the concept.

BTW I have not tested the cases you have mentioned. Here are the some
sample test cases (I haven't paste complete test cases I have used)

CREATE TABLE population_tbl (country varchar, male NUMERIC, female NUMERIC);

INSERT INTO population_tbl values ('Australia',1,100);
INSERT INTO population_tbl values ('Denmark',2,200);
INSERT INTO population_tbl values ('Germany',3,300);
INSERT INTO population_tbl values ('Netherlands',4,400);
INSERT INTO population_tbl values ('United States',5,500);
INSERT INTO population_tbl values ('Pakistan',6,600);

--GROUPING SET
SELECT country,male,female FROM population_tbl GROUP BY GROUPING
SETS(country,male,female,());
SELECT country,male,female FROM population_tbl GROUP BY GROUPING
SETS((country,male,female));
SELECT country,male,female,sum(male) FROM population_tbl GROUP BY
GROUPING SETS(country,(male,female));

SELECT country,male,female FROM population_tbl GROUP BY ALL GROUPING
SETS(country,male,female,());
SELECT country,male,female FROM population_tbl GROUP BY ALL GROUPING
SETS((country,male,female));
SELECT country,male,female,sum(male) FROM population_tbl GROUP BY ALL
GROUPING SETS(country,(male,female));

SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
GROUPING SETS(country,male,female,());
SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
GROUPING SETS((country,male,female));
SELECT country,male,female,sum(male) FROM population_tbl GROUP BY
DISTINCT GROUPING SETS(country,(male,female));


SELECT grouping(country),country,male,female FROM population_tbl GROUP
BY GROUPING SETS(country,male,female,());
SELECT grouping(country),country,male,female FROM population_tbl GROUP
BY GROUPING SETS((country,male,female));
SELECT grouping(country),country,male,female,sum(male) FROM
population_tbl GROUP BY GROUPING SETS(country,(male,female));

SELECT grouping(country),country,male,female FROM population_tbl GROUP
BY ALL GROUPING SETS(country,male,female,());
SELECT grouping(country),country,male,female FROM population_tbl GROUP
BY ALL GROUPING SETS((country,male,female));
SELECT grouping(country),country,male,female,sum(male) FROM
population_tbl GROUP BY ALL GROUPING SETS(country,(male,female));

SELECT grouping(country),country,male,female FROM population_tbl GROUP
BY DISTINCT GROUPING SETS(country,male,female,());
SELECT grouping(country),country,male,female FROM population_tbl GROUP
BY DISTINCT GROUPING SETS((country,male,female));
SELECT grouping(country),country,male,female,sum(male) FROM
population_tbl GROUP BY DISTINCT GROUPING SETS(country,(male,female));

SELECT grouping_id(country),country,male,female FROM population_tbl
GROUP BY GROUPING SETS(country,male,female,());
SELECT grouping_id(country),country,male,female FROM population_tbl
GROUP BY GROUPING SETS((country,male,female));
SELECT grouping_id(country),country,male,female,sum(male) FROM
population_tbl GROUP BY GROUPING SETS(country,(male,female));

SELECT grouping_id(country),country,male,female FROM population_tbl
GROUP BY ALL GROUPING SETS(country,male,female,());
SELECT grouping_id(country),country,male,female FROM population_tbl
GROUP BY ALL GROUPING SETS((country,male,female));
SELECT grouping_id(country),country,male,female,sum(male) FROM
population_tbl GROUP BY ALL GROUPING SETS(country,(male,female));

SELECT grouping_id(country),country,male,female FROM population_tbl
GROUP BY DISTINCT GROUPING SETS(country,male,female,());
SELECT grouping_id(country),country,male,female FROM population_tbl
GROUP BY DISTINCT GROUPING SETS((country,male,female));
SELECT grouping_id(country),country,male,female,sum(male) FROM
population_tbl GROUP BY DISTINCT GROUPING SETS(country,(male,female));

--Neg: SELECT country,male,female,sum(male) FROM population_tbl GROUP
BY GROUPING SETS((country),(male),female,());

--ROLLUP
SELECT country,male,female FROM population_tbl GROUP BY
ROLLUP(country,male,female);
SELECT country,male,female FROM population_tbl GROUP BY
ROLLUP(country,(male,female));
SELECT country,male,female FROM population_tbl GROUP BY
ROLLUP(country,(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY
ROLLUP((country),(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY
ROLLUP((country,male),(female));

SELECT country,male,female FROM population_tbl GROUP BY ALL
ROLLUP(country,male,female);
SELECT country,male,female FROM population_tbl GROUP BY ALL
ROLLUP(country,(male,female));
SELECT country,male,female FROM population_tbl GROUP BY ALL
ROLLUP(country,(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY ALL
ROLLUP((country),(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY ALL
ROLLUP((country,male),(female));

SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
ROLLUP(country,male,female);
SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
ROLLUP(country,(male,female));
SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
ROLLUP(country,(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
ROLLUP((country),(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
ROLLUP((country,male),(female));

--CUBE
SELECT country,male,female FROM population_tbl GROUP BY
CUBE(country,male,female);
SELECT country,male,female FROM population_tbl GROUP BY
CUBE(country,(male,female));
SELECT country,male,female FROM population_tbl GROUP BY
CUBE(country,(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY
CUBE((country),(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY
CUBE((country,male),(female));

SELECT country,male,female FROM population_tbl GROUP BY ALL
CUBE(country,male,female);
SELECT country,male,female FROM population_tbl GROUP BY ALL
CUBE(country,(male,female));
SELECT country,male,female FROM population_tbl GROUP BY ALL
CUBE(country,(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY ALL
CUBE((country),(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY ALL
CUBE((country,male),(female));

SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
CUBE(country,male,female);
SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
CUBE(country,(male,female));
SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
CUBE(country,(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
CUBE((country),(male),(female));
SELECT country,male,female FROM population_tbl GROUP BY DISTINCT
CUBE((country,male),(female));



--Problems
--------

1 - ORDER BY CLAUSE is not working after the patch

--After Patch
----------------

postgres=# SELECT country,male,female FROM population_tbl order by male DESC;
    country    | male | female
---------------+------+--------
 Australia     |    1 |    100
 Denmark       |    2 |    200
 Germany       |    3 |    300
 Netherlands   |    4 |    400
 United States |    5 |    500
 Pakistan      |    6 |    600
(6 rows)


--Before patch
-------------------

postgres=# SELECT country,male,female FROM population_tbl order by male DESC;
    country    | male | female
---------------+------+--------
 Pakistan      |    6 |    600
 United States |    5 |    500
 Netherlands   |    4 |    400
 Germany       |    3 |    300
 Denmark       |    2 |    200
 Australia     |    1 |    100
(6 rows)


Some Minor code observations
----------------------
1 - IMHO we should use enum instead of #define

 i.e
#define CUBE_OP			1
#define ROLLUP_OP		2
#define FUNCCALL_OP		3


-- 
   Ibrar Ahmed
   EnterpriseDB   http://www.enterprisedb.com

Responses

pgsql-hackers by date

Next:From: A. KretschmerDate: 2008-11-24 11:02:08
Subject: blatantly a bug in the documentation
Previous:From: Zdenek KotalaDate: 2008-11-24 09:40:44
Subject: Re: Minor race-condition problem during database startup

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group