Re: WITHIN GROUP patch

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