Thread: Re: [PATCH] Add array_reverse() function

On Mon, Oct 21, 2024 at 2:36 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi,
>
> Recently I wanted to call array_reverse() and discovered that we still
> don't have it. I'm not the first one who encountered this limitation.
> array_reverse() was requested at least since 2009 [1] and the
> workaround on PostgreSQL Wiki is dated 2010 [2].
>
> The proposed patch adds this function. Only the first dimension of the
> array is reversed, for consistency with the existing functions such as
> array_shuffle() [3].
>
> Examples:
>
> =# SELECT array_reverse(ARRAY[1,2,3,NULL]);
>  array_reverse
> ---------------
>  {NULL,3,2,1}
>
> =# SELECT array_reverse(ARRAY[[1,2],[3,4],[5,6]]);
>     array_reverse
> ---------------------
>  {{5,6},{3,4},{1,2}}
>
> Thoughts?

Looks useful. Glancing quickly at the code I have a comment

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


+ /*
+ * There is no point in reversing empty arrays or arrays with less than
+ * two items.
+ */
+ if (ARR_NDIM(array) < 1 || ARR_DIMS(array)[0] < 2)
+ PG_RETURN_ARRAYTYPE_P(array);

But it returns the input array as is. I think it should at least make
a new copy of input array.

--
Best Wishes,
Ashutosh Bapat

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
> On Mon, Oct 21, 2024 at 2:36 PM Aleksander Alekseev
> <aleksander@timescale.com> wrote:
> + /*
> + * There is no point in reversing empty arrays or arrays with less than
> + * two items.
> + */
> + if (ARR_NDIM(array) < 1 || ARR_DIMS(array)[0] < 2)
> +     PG_RETURN_ARRAYTYPE_P(array);

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

            regards, tom lane

On Tuesday, October 22, 2024 16:27 MSK, Aleksander Alekseev <aleksander@timescale.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


--