[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

On Mon, Oct 21, 2024, at 11:06, Aleksander Alekseev 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].

+1

I've needed this many times.

/Joel

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

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]/messages/by-id/CAJ7c6TO3X88dGd8C4Tb-Eq2ZDPz+9mP+KOwdzK_82BEz_cMPZg@mail.gmail.com 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]: /messages/by-id/CAJ7c6TO3X88dGd8C4Tb-Eq2ZDPz+9mP+KOwdzK_82BEz_cMPZg@mail.gmail.com

--
Best regards,
Aleksander Alekseev

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]/messages/by-id/CAJ7c6TO3X88dGd8C4Tb-Eq2ZDPz+9mP+KOwdzK_82BEz_cMPZg@mail.gmail.com 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]: /messages/by-id/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.

On Wed, Oct 30, 2024 at 10:11:20PM +0300, Пополитов Владлен wrote:

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.

We are talking about 20 lines copied over to reverse the order of
these elements that require their own inner for the element lookups
and manipulations. FWIW, I can live with this burden rather than
invent a routine that encapsulates both as this has also the
disadvantage of hiding more internals across more layers.

The abstraction with array_reverse_n() is unnecessary, but you have an
argument with the consistency and the shuffling inner routine.
Perhaps somebody will reuse that to invent a function that works on a
subset, where the comment on top array_reverse_n() about the type
caching still works. Anyway, no objections to what the patch defines
and does.

So it looks rather OK seen from here, as you are proposing.
--
Michael

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: tested, failed
Documentation: tested, failed

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

On Thu, Oct 31, 2024 at 11:10:09AM +0900, Michael Paquier wrote:

So it looks rather OK seen from here, as you are proposing.

After a second lookup, bumped CATALOG_VERSION_NO, then applied.
--
Michael