Hi,
> Based on the previous discussion, I split it into two patches in V8.
>
> 0001 is the general sort part without `is_ascending` or `nulls_first`,
> the sort order is determined by the "<" operator of the element type.
> It also cached the type entry of both eletyp and the corresponding
> array type.
>
> 0002 adds the `is_ascending` and `nulls_first` part, it now uses
> two boolean parameters instead of parsing one text parameter.
Thanks for the update patch set. Here are some comments.
0001:
> +{ oid => '8810', descr => 'sort array',
> + proname => 'array_sort', provolatile => 'v', prorettype => 'anyarray',
> + proargtypes => 'anyarray', prosrc => 'array_sort'},
I would expect that array_sort() should be IMMUTABLE. Is there a
reason for it to be VOLATILE?
> + <function>array_sort</function> ( <type>anyarray</type> <optional>
> COLLATE <replaceable>collation_name</replaceable> </optional>)
> + <returnvalue>anyarray</returnvalue>
It seems to me that the part about using COLLATE should be moved
below, to the description / examples section, since it's not part of
the function signature.
Also the description should be more specific about how NULLs are
sorted. NULLs also should be covered by tests.
0002:
> <parameter>is_ascending</parameter>
I really believe this name is not the best one. I suggest using
`reverse => true`. `nulls_first` is OK.
> +Datum
> +array_sort_order(PG_FUNCTION_ARGS)
> +{
> + return array_sort(fcinfo);
> +}
> +
> +Datum
> +array_sort_order_nulls_first(PG_FUNCTION_ARGS)
> +{
> + return array_sort(fcinfo);
> +}
Any reason not to specify array_sort in pg_proc.dat?
The tests cover is_ascending => true | false, which is OK, but only
(is_ascending = true, nulls_first => true) and (is_ascending => false,
nulls_fist => false). For the case when both optional arguments are
specified you have to test at least 4 combinations.
--
Best regards,
Aleksander Alekseev
