Aggregate ORDER BY patch
in [PgSql]
|
From: Hitoshi Harada on 15 Nov 2009 23:49 2009/11/16 Andrew Gierth <andrew(a)tao11.riddles.org.uk>: >>>>>> "Hitoshi" == Hitoshi Harada <umi.tanuki(a)gmail.com> writes: > > >> What case exactly would you consider an error? When an order by > >> expression references a lower (more deeply nested) query level > >> than any of the actual arguments? > > Hitoshi> It's only that I felt not intuitive. To me, arguments are > Hitoshi> regarded as aggregate's "member" while ORDER BY clause > Hitoshi> expressions didn't hit me. If it's only me, no > Hitoshi> problem. Maybe additional document in #syntax-aggregates > Hitoshi> will do. > > How about: > > ... But an exception occurs if the aggregate's arguments > (including any <literal>ORDER BY</> clause) contain only > outer-level variables: the aggregate then belongs to the nearest > such outer level, ... Reasonable. Thank you. > Hitoshi> I don't have much experiences in VIEW systems, but isn't it > Hitoshi> enough to let "order by x" omitted? "select count(distinct x > Hitoshi> order by x) from table" means the same as "select > Hitoshi> count(distinct x) from table" currently. ruleutils can > Hitoshi> handle it if distinct expressions are equal to order by > Hitoshi> expressions. > > That doesn't work in more complex cases. For example, the user might > specify aggfunc(distinct x,y order by x) (not caring about the relative > order of y) but the code will still turn that internally into > aggfunc(distinct x,y order by x,y). It's necessary to be able to recover > what the user originally entered, which means needing to be able to > distinguish both of those cases from aggfunc(distinct x,y). > With Tom's comment, this issue is closed now with some hope that author will see if new code can be shared with traditional code once more. So I guess all of my review comments are get done. Could you update your patch with doc patch in it? After that I'll test it again and will mark this as "Ready for Committer" if no objection nor no problem found. Regards, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Andrew Gierth on 16 Nov 2009 00:04 >>>>> "Tom" == Tom Lane <tgl(a)sss.pgh.pa.us> writes: >> What about error handling? If the user specifies agg(distinct x) >> where x is not sortable, do we leave it to the planner to detect >> that (which means not reporting the error position?) Tom> Well, at the moment there's only going to be a sort-based Tom> implementation, so I don't object to throwing an error for that Tom> as soon as possible. OTOH I wouldn't recommend expending a lot Tom> of code to do it there. I would hope that most of the parser's Tom> work for this can be shared with the existing support for Tom> query-level ORDER BY/DISTINCT. The code already uses transformSortClause for most of the work, but reusing the existing code for DISTINCT would have required more refactoring than I was happy with, because transformDistinct etc. all have error message text which is specific to SELECT DISTINCT etc. Let's see how it falls out in the next patch. -- Andrew. -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Andrew Gierth on 30 Nov 2009 05:34 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 -- Andrew (irc:RhodiumToad)
From: Hitoshi Harada on 2 Dec 2009 13:54 2009/11/30 Andrew Gierth <andrew(a)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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Alvaro Herrera on 3 Dec 2009 14:52
Hitoshi Harada escribi�: > 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. AFAICS the catversion bump is needed because of the change in a parser node. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |