Re: Aggregate ORDER BY patch

2009/11/30 Andrew Gierth <andrew@tao11.riddles.org.uk>:
> Updated version of the aggregate order by patch.
>
> Includes docs + regression tests all in the same patch.
>
> Changes:
>
>  - removed SortGroupClause.implicit as per review comments,
>    replacing it with separate lists for Aggref.aggorder and
>    Aggref.aggdistinct.
>
>  - Refactored in order to move the bulk of the new parse code
>    out of ParseFuncOrColumn which was already quite big enough,
>    into parse_agg.c
>
>  - fixed a bug with incorrect deparse in ruleutils (and added a
>    bunch of regression tests for deparsing and view usage)
>
>  - added some comments

It seems good to me. Everything that was pointed in the previous
review was fixed, as well as sufficient comments are added. It applies
very cleanly against HEAD and compiles without error/warning.

I found only trivial favors such like that a blank line is added
around line 595 in the patch, and "proj" in peraggstate sounds a
little weird to me because of surrounding "evaldesc" and "evalslot"
("evalproj" seems better to me). Also catversion update doesn't mean
anything for this feature. But these are not what prevent it from
review by a committer. So, although I'm going to look more on this
patch, I mark this item as "Ready for Committer" for now.


Regards,

--
Hitoshi Harada