Re: WITHIN GROUP patch

On Tue, Oct 15, 2013 at 4:29 PM, Vik Fearing <vik.fearing@dalibo.com> wrote:
> On 10/09/2013 04:19 PM, Pavel Stehule wrote:
>
>
>> I checked a conformance with ANSI SQL - and I didn't find any issue.
>>
>> I found so following error message is not too friendly (mainly because
>> this functionality will be new)
>>
>> postgres=# select dense_rank(3,3,2) within group (order by num desc, odd)
>> from test4;
>> ERROR:  Incorrect number of arguments for hypothetical set function
>> LINE 1: select dense_rank(3,3,2) within group (order by num desc, od...
>>                ^
>> postgres=# select dense_rank(3,3,2) within group (order by num desc) from
>> test4;
>> ERROR:  Incorrect number of arguments for hypothetical set function
>> LINE 1: select dense_rank(3,3,2) within group (order by num desc) fr...
>>                ^
>> postgres=# select dense_rank(3,3) within group (order by num desc) from
>> test4;
>> ERROR:  Incorrect number of arguments for hypothetical set function
>> LINE 1: select dense_rank(3,3) within group (order by num desc) from...
>>                ^
>> postgres=# select dense_rank(3,3) within group (order by num desc, num)
>> from test4;
>>  dense_rank
>> ------------
>>           3
>> (1 row)
>>
>> Probably some hint should be there?
>>
>>
>
> In addition to Pavel's review, I have finally finished reading the patch.
> Here are some notes, mainly on style:
>
> First of all, it no longer compiles on HEAD because commit
> 4d212bac1752e1bad6f3aa6242061c393ae93a0a stole oid 3968.  I modified that
> locally to be able to continue my review.
>
> Some of the error messages do not comply with project style.  That is, they
> begin with a capital letter.
>
> Ordered set functions cannot have transition functions
> Ordered set functions must have final functions
> Invalid argument types for hypothetical set function
> Invalid argument types for ordered set function
> Incompatible change to aggregate definition
> Too many arguments to ordered set function
> Ordered set finalfns must not be strict
> Cannot have multiple ORDER BY clauses with WITHIN GROUP
> Cannot have DISTINCT and WITHIN GROUP together
>
> Incorrect number of arguments for hypothetical set function
> Incorrect number of direct arguments to ordered set function %s
>
> And in pg_aggregate.c I found a comment with a similar problem that doesn't
> match its surrounding code:
> Oid            transsortop = InvalidOid;  /* Can be omitted */
>
> I didn't find any more examples like that, but I did see several block
> comments that weren't complete sentences whereas I think they should be.
> Also a lot of the code comments say "I" and I don't recall seeing that
> elsewhere.  I may be wrong, but I would prefer if they were more neutral.
>
> The documentation has a number of issues.
>
> collateindex.pl complains of duplicated index entries for PERCENTILE
> CONTINUOUS and PERCENTILE DISCRETE.  This is because the index markup is
> used for both overloaded versions.  This is the same mistake Bruce made and
> then corrected in commit 5dcc48c2c76cf4b2b17c8e14fe3e588ae0c8eff3.
>
> "if there are multiple equally good result" should have an s on the end in
> func.sgml.
>
> Table 9-49 has an extra empty column.  That should either be removed, or
> filled in with some kind of comment text like other similar tables.
>
> Apart from that, it looks good.  There is some mismatched coding styles in
> there but the next run of pgindent should catch them so it's no big deal.
>
> I haven't yet exercised the actual functionality of the new functions, nor
> have I tried to create my own.  Andrew alerted me to a division by zero bug
> in one of them, so I'll be looking forward to catching that.
>
> So, more review to come.
>
> --
> Vik

Hi All,

Please find attached our latest version of the patch. This version
fixes the issues pointed out by the reviewers.

Regards,

Atri

--
Regards,

Atri
l'apprenant