>> 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? >> > > I probably got the word "sort" from the function targetIsInSortList, which > expects a list of SortGroupClause. I've renamed the function to > sortlist_is_unique_on_restrictinfo() and renamed the sortclause parameter to > sortlist. Hopefully will reduce confusion about it being an ORDER BY clause > a bit more. I think sortgroupclauselist might be just a bit too long. What > do you think?
OK, perhaps I should be clearer. The word "sort" here seems completely misplaced and we should be using a more accurately descriptive term. It's slightly more than editing to rename things like that, so I'd prefer you cam up with a better name.
hmm, I do see what you mean and understand the concern, but I was a bit stuck on the fact it is a list of SortGroupClause after all. After a bit of looking around the source I found a function called grouping_is_sortable which seems to be getting given ->groupClause and ->distinctClause in a few places around the source. I've ended up naming the function groupinglist_is_unique_on_restrictinfo, but I can drop the word "list" off of that if that's any better? I did have it named clauselist_is_unique_on_restictinfo for a few minutes, but then I noticed that this was not very suitable since the calling function uses the variable name clause_list for the restrictinfo list, which made it even more confusing.
Attached is a delta patch between version 1.2 and 1.3, and also a completely updated patch.
Did you comment on the transitive closure question? Should we add a test for that, whether or not it works yet?
In my previous email.
I could change the the following to use c.id in the targetlist and group by clause, but I'm not really sure it's testing anything new or different.
