Re: [PATCH] Add array_reverse() function

On Tuesday, October 22, 2024 16:27 MSK, Aleksander Alekseev <aleksander(at)timescale(dot)com> wrote:

 
Hi everyone,

Thanks for the feedback!

> > But it returns the input array as is. I think it should at least make
> > a new copy of input array.
>
> I don't think that's really necessary. We have other functions that
> will return an input value unchanged without copying it. A
> longstanding example is array_larger. Also, this code looks to be
> copied from array_shuffle.

It was indeed copied from array_shuffle().

Makes me wonder if we should have an utility function which would
contain common code for array_shuffle() and array_reverse().
Considering inconveniences such an approach caused in case of common
code for text and bytea types [1] I choose to copy the code and modify
it for the task.

> +
> + /* If the target array is empty, exit fast */
> + if (ndim < 1 || dims[0] < 1)
> + return construct_empty_array(elmtyp);
>
> This is taken care by the caller, at least the ndim < 1 case.

Agree. Here is the corrected patch.

[1]: https://postgr.es/m/CAJ7c6TO3X88dGd8C4Tb-Eq2ZDPz+9mP+KOwdzK_82BEz_cMPZg@mail.gmail.com

--
Best regards,
Aleksander Alekseev
 
Hi!
 
Content.
The proposed function waited long to be implemented and it will be very
useful. Personally I used slow workaround, when I needed the reverse of
the array.
 
Run.
This patch applies cleanly to HEAD. All regression tests pass
successfully against the patch.
 
Format.
The code formatted according to The Code Guidelines
 
Documentation.
The documentation is updated, the description of the function is added.
From my point of view, it would be better to mention, that function returns
updated array (does not updates it in place, as a reader can understand),
but other array returning functions in the documentation has the same 
style (silently assume: reverse == return reversed array).
 
Conclusion.
+1 for commiter review
 
Best regards,
 
Vladlen Popolitov
postgrespro.com

-- 
 Best regards,

Vladlen Popolitov.