Re: [Review] Grouping Sets patch

Hello,

thank you, for you review. Grouping Sets is nice feature, but this
patch patch has some issues, and I don't expect commiting into 8.4. I
sent it, because it is interaction with window function's patch and
(maybe) some code should be shared - and this information should be
usefull for other developers (commiters). Actually almoust all of
planner part should be rewriten. Now GS is in separate branch than
classic GROUP BY clause. I am sure, so GROUP BY is special case of GS
- it means relativelly big changes in GROUP BY planner part. So now I
am wainting on commiting window functions - and then I'll continue.

Regards
Pavel Stehule



2008/11/24 Ibrar Ahmed <ibrar.ahmad@gmail.com>:
> 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
>