On 22 June 2014 12:51, Simon Riggs <si...@2ndquadrant.com> wrote: > Looks good on initial look.
Tests 2 and 3 seem to test the same thing. There are no tests which have multiple column clauselist/sortlists, nor tests for cases where the clauselist is a superset of the sortlist. Test comments should refer to "join removal" rather than "optimization" because we may forget which optimization they are there to test. It's not clear to me where you get the term "sortclause" from. This is either the groupclause or distinctclause, but in the test cases you provide this shows this has nothing at all to do with sorting since there is neither an order by or a sorted aggregate anywhere near those queries. Can we think of a better name that won't confuse us in the future? The comment "Since a constant only has 1 value the existence of one here will + * not cause any duplication of the results. We'll simply ignore it!" would be better as "We can ignore constants since they have only one value and don't affect uniqueness of results". The comment "XXX is this comment still true??" can be removed since its just a discussion point. The comment beginning "Currently, we only know how to remove left..." has rewritten a whole block of text just to add a few words in the middle. We should rewrite the comment so it changes as few lines as possible. Especially when that comment is going to be changed again with your later patches. Better to have it provide a bullet point list of things we know how to remove, so we can just add to it later. Still looks good, other than the above. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers