Support for array_remove and array_replace functions

On Thu, Jun 14, 2012 at 4:41 AM, Marco Nenciarini <
marco.nenciarini@2ndquadrant.it> wrote:

Hi,

following Gabriele's email regarding our previous patch on "Foreign
Key Arrays"[1], I am sending a subset of that patch which includes only
two array functions which will be needed in that patch: array_remove
(limited to single-dimensional arrays) and array_replace.

The patch includes changes to the documentation.

Hi, I've been reviewing this patch.

Good documentation, and regression tests. The code looked fine but I didn't
care for the code duplication between array_replace and array_remove so I
merged those into a helper function, array_replace_internal(). Thoughts?

Other than that it all looks good to me.

On 30/06/2012 04:16, Alex Hunsaker wrote:

Hi, I've been reviewing this patch.

Good documentation, and regression tests. The code looked fine but I
didn't care for the code duplication between array_replace and
array_remove so I merged those into a helper function,
array_replace_internal(). Thoughts?

It looks reasonable.

There was a typo in array_replace which was caught by regression tests.
I've fixed the typo and changed a comment in array_replace_internal.

Patch v3 attached.

Regards,
Marco

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it

On Sat, Jun 30, 2012 at 3:28 PM, Marco Nenciarini
<marco.nenciarini@2ndquadrant.it> wrote:

On 30/06/2012 04:16, Alex Hunsaker wrote:

Hi, I've been reviewing this patch.

Good documentation, and regression tests. The code looked fine but I
didn't care for the code duplication between array_replace and
array_remove so I merged those into a helper function,
array_replace_internal(). Thoughts?

It looks reasonable.

There was a typo in array_replace which was caught by regression tests.
I've fixed the typo and changed a comment in array_replace_internal.

Patch v3 attached.

Looks good to me, marked ready for commiter.

Marco Nenciarini <marco.nenciarini@2ndquadrant.it> writes:

Patch v3 attached.

I'm looking at this patch now. The restriction of array_remove to
one-dimensional arrays seems a bit annoying. I see the difficulty:
if the input is multi-dimensional then removing some elements could
lead to a non-rectangular array, which isn't supported. However,
that could be dealt with by decreeing that the *result* is
one-dimensional and of the necessary length, regardless of the
dimensionality of the input.

I'm not actually certain whether that's a better definition or not.
But one less error case seems like generally a good thing.
Comments?

regards, tom lane

On Wed, Jul 11, 2012 at 9:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Marco Nenciarini <marco.nenciarini@2ndquadrant.it> writes:

Patch v3 attached.

I'm looking at this patch now. The restriction of array_remove to
one-dimensional arrays seems a bit annoying. I see the difficulty:
if the input is multi-dimensional then removing some elements could
lead to a non-rectangular array, which isn't supported. However,
that could be dealt with by decreeing that the *result* is
one-dimensional and of the necessary length, regardless of the
dimensionality of the input.

Makes sense to me. +1

The other option ISTM is to replace removed entries with NULL-- which
I don't really like.

Alex Hunsaker <badalex@gmail.com> writes:

On Wed, Jul 11, 2012 at 9:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm looking at this patch now. The restriction of array_remove to
one-dimensional arrays seems a bit annoying. I see the difficulty:
if the input is multi-dimensional then removing some elements could
lead to a non-rectangular array, which isn't supported. However,
that could be dealt with by decreeing that the *result* is
one-dimensional and of the necessary length, regardless of the
dimensionality of the input.

Makes sense to me. +1

The other option ISTM is to replace removed entries with NULL-- which
I don't really like.

Well, you can do that with array_replace, so I don't see a need to
define array_remove that way.

regards, tom lane

On Jul 11, 2012, at 11:53 AM, Alex Hunsaker <badalex@gmail.com> wrote:

On Wed, Jul 11, 2012 at 9:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Marco Nenciarini <marco.nenciarini@2ndquadrant.it> writes:

Patch v3 attached.

I'm looking at this patch now. The restriction of array_remove to
one-dimensional arrays seems a bit annoying. I see the difficulty:
if the input is multi-dimensional then removing some elements could
lead to a non-rectangular array, which isn't supported. However,
that could be dealt with by decreeing that the *result* is
one-dimensional and of the necessary length, regardless of the
dimensionality of the input.

Makes sense to me. +1

+1 from me, too.

...Robert

Marco Nenciarini <marco.nenciarini@2ndquadrant.it> writes:

Patch v3 attached.

Applied with mostly-but-not-entirely cosmetic adjustments.

I left array_remove throwing error for multi-dimensional arrays for
the moment, because I realized that changing the dimensionality as
I suggested would conflict with the optimization to return the original
array if there were no matches. I don't think we'd want the definition
to read "multidimensional arrays are changed to one dimension, but only
if at least one element is removed" --- that's getting a little too
weird. If anyone's really hot to make it work on multi-D arrays, we
could consider disabling that optimization; it's not clear to me that
it's worth a lot. But for now I'm willing to stick with the
throw-an-error approach.

regards, tom lane