Re: general purpose array_sort

Hi Jian,

On Mon, Sep 30, 2024 at 11:13 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> On Mon, Sep 30, 2024 at 1:01 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
> >
> > > I would suggest accepting:
> > > asc
> > > desc
> > > asc nulls first
> > > asc nulls last *
> > > desc nulls first *
> > > desc nulls last
> > >
> > > As valid inputs for "dir" - and that the starred options are the defaults when null position is omitted.
> > >
> > > In short, mimic create index.
> > >
> > > David J.
> > >
> >
> > PFA v3 with David's suggestion addressed.
> >
>
> I think just adding 2 bool arguments (asc/desc, nulls last/not nulls
> last) would be easier.
Yeah, this would be easier, it's just the intarray module use
the direction parameter, I keep it here for the same user
experience, I don't insist if some committer thinks 2 bool arguments
would be a better option.

> but either way, (i don't have a huge opinion)
> but document the second argument, imagine case
> SELECT array_sort('{a,B}'::text[] , E'aSc NulLs LaST \t\r\n');
> would be tricky?

The case you provide should give the correct results, but
I doubt users will do this.
I'm not good at document wording, so you might give me some help
with the document part.

>
>
> errmsg("multidimensional arrays sorting are not supported")));
> write a sql test to trigger the error message that would be great.
>
> you can add two or one example to collate.icu.utf8.sql to demo that it
> actually works with COLLATE collation_name
> like:
> SELECT array_sort('{a,B}'::text[] COLLATE case_insensitive);
> SELECT array_sort('{a,B}'::text[] COLLATE "C");
>
Fixed.

>
> #define WHITESPACE " \t\n\r"
> you may also check function scanner_isspace
>
Fixed.

>
> + typentry = (TypeCacheEntry *) fcinfo->flinfo->fn_extra;
> + if (typentry == NULL || typentry->type_id != elmtyp)
> + {
> + typentry = lookup_type_cache(elmtyp, sort_asc ? TYPECACHE_LT_OPR :
> TYPECACHE_GT_OPR);
> + fcinfo->flinfo->fn_extra = (void *) typentry;
> + }
> you need to one-time check typentry->lt_opr or typentry->gt_opr exists?
> see CreateStatistics.
> /* Disallow data types without a less-than operator */
> type = lookup_type_cache(attForm->atttypid, TYPECACHE_LT_OPR);
> if (type->lt_opr == InvalidOid)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("column \"%s\" cannot be used in
> statistics because its type %s has no default btree operator class",
> attname, format_type_be(attForm->atttypid))));

I added an Assert for this part, not sure if that is enough.

--
Regards
Junwang Zhao