[Review] Grouping Sets patch - Mailing list 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 |    100Denmark       |    2 |    200Germany       |    3 |
300Netherlands  |    4 |    400United States |    5 |    500Pakistan      |    6 |    600
 
(6 rows)


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

postgres=# SELECT country,male,female FROM population_tbl order by male DESC;   country    | male | female
---------------+------+--------Pakistan      |    6 |    600United States |    5 |    500Netherlands   |    4 |
400Germany      |    3 |    300Denmark       |    2 |    200Australia     |    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