Re: general purpose array_sort

On Sun, Nov 03, 2024 at 11:33:05AM +0800, Junwang Zhao wrote:
> Rebase needed due to array_reverse committed, PFA v11.

There has been another conflict since you have posted this version
(noticed that after my business in 027124a872d7). I have looked at
0001.

+ if (ARR_NDIM(array) < 1)
+ PG_RETURN_ARRAYTYPE_P(array);
There is no point in doing a sort if the array has only one element.
You can add a check based on "ARR_DIMS(array)[0] < 2" to achieve that.

+ typentry = lookup_type_cache(elmtyp, TYPECACHE_LT_OPR);
+ if (!OidIsValid(typentry->lt_opr))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_FUNCTION),
+ errmsg("could not identify ordering operator for type %s",
+ format_type_be(elmtyp))));

The patch introduces two error paths based on the fact that ordering
operators could not be found depending on a data type that lacks the
ordering operator and the array ordering operator part. It is right
to issue an error if these are lacking, like the various stats paths.
Should we have some regression tests with specific data types for
these errors, though? The stats paths don't care much about these
error cases, but it does not mean that we should not care about them.
In short, let's have negative test coverage if we can.

+typedef struct ArraySortCachedInfo
+{
+ TypeCacheEntry *typentry;
+ TypeCacheEntry *array_typentry;
+} ArraySortCachedInfo;

Let's put that at the top of the file, with a comment about how it
links to array_sort() for the caching with fn_extra. Let's also
document the meaning of the fields.

FWIW, I am confused by this implementation, where you have to allocate
the two TypeCacheEntry because of the fact that you have to deal with
the 1-dimension case and the multi-dimension case. In the context of
a single function call, why do you need both typentry and
array_typentry, actually? Wouldn't it be enough to use one typentry
that points to the typcache, meaning that you don't really need to use
the extra business with fn_mcxt, no? If you require both (because I
may be wrong), perhaps you should have a regression test that's able
to break when removing array_typentry, changing the code to only rely
on typentry. Note: I have just removed array_typentry in a quick
test, current coverage was happy about it. Feel free to prove me
wrong.

Agreed that the function should be immutable. The results are fixed
depending on the input even with the COLLATE clauses appended.

Let's add something when there is only one element in the first
dimension of the array, say two cases one with an int and one with an
array of ints like:
SELECT array_sort('{1}'::int[]);
SELECT array_sort('{{1}}'::int[]);
--
Michael