Re: general purpose array_sort

Hi Aleksander,

On Tue, Oct 29, 2024 at 12:48 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> 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?

I saw Jian's reply about this, but I tend to agree with you, so remove
provolatile => 'v'.

>
> > +        <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.

Agree, fixed with my own words, help needed with the wording.

>
> Also the description should be more specific about how NULLs are
> sorted. NULLs also should be covered by tests.

Fixed.

>
> 0002:
>
> > <parameter>is_ascending</parameter>
>
> I really believe this name is not the best one. I suggest using
> `reverse => true`. `nulls_first` is OK.

Not sure about this, I think `is_ascending` has a more precise
meaning, while `reverse` doesn't show any hint about ascending or
descending, just keep it right now, let's see others' opinions.

>
> > +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?

It is specified in 0001 (see oid => '8810').

>
> 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.

The omitted two is the same as the two with two parameters specified,
anyway, add all 4 cases in v9.

>
> --
> Best regards,
> Aleksander Alekseev



-- 
Regards
Junwang Zhao