Re: [PATCH] Introduce array_shuffle() and array_sample()

Am 24.07.22 um 10:15 schrieb Fabien COELHO:
>
> My 0,02€ about this patch:

Thank you for your feedback. I attached a patch, that addresses most of
your points.

> Use (void) when declaring no parameters in headers or in functions.

Done.

> Should the exchange be skipped when i == k?

The additional branch is actually slower (on my machine, tested with an
10M element int array) for 1D-arrays, which i assume is the most common
case. Even with a 2D-array with a subarray size of 1M there is no
difference in execution time. i == k should be relatively rare for large
arrays, given a good prng.

> I do not see the point of having *only* inline functions in a c file.
> The external functions should not be inlined?

Done.

> The random and array shuffling functions share a common state. I'm
> wondering whether it should ot should not be so. It seems ok, but then
> ISTM that the documentation suggests implicitely that setseed applies to
> random() only, which is not the case anymore after the patch.

I really like the idea of a prng state owned by the user, that is used
by all user facing random functions, but see that their might be
concerns about this change. I would update the setseed() documentation,
if this proposal is accepted. Do you think my patch should already
include the documentation update?

> If more samples are required than the number of elements, it does not
> error out. I'm wondering whether it should.

The second argument to array_sample() is treated like a LIMIT, rather
than the actual number of elements. I updated the documentation. My
use-case is: take max random items from an array of unknown size.

> Also, the sampling should not return its result in order when the number
> of elements required is the full array, ISTM that it should be shuffled
> there as well.

You are the second person asking for this. It's done. I am thinking
about ditching array_sample() and replace it with a second signature for
array_shuffle():

array_shuffle(array anyarray) -> anyarray
array_shuffle(array anyarray, limit int) -> anyarray

What do you think?

> I must say that without significant knowledge of the array internal
> implementation, the swap code looks pretty strange. ISTM that the code
> would be clearer if pointers and array references style were not
> intermixed.

Done. Went with pointers.

> Maybe you could add a test with a 3D array? Some sample with NULLs?

Done.

> Unrelated: I notice again that (postgre)SQL does not provide a way to
> generate random integers. I do not see why not. Should we provide one?

Maybe. I think it is outside the scope of this patch.

Thank you, Martin