Re: Support for array_remove and array_replace functions

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.

Cheers,
Marco

[1] http://archives.postgresql.org/message-id/4FD8F422.40709%402ndQuadrant.it

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco(dot)nenciarini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

On Thu, Jun 14, 2012 at 4:41 AM, Marco Nenciarini <
marco(dot)nenciarini(at)2ndquadrant(dot)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(dot)nenciarini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

On Sat, Jun 30, 2012 at 3:28 PM, Marco Nenciarini
<marco(dot)nenciarini(at)2ndquadrant(dot)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(dot)nenciarini(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marco Nenciarini <marco(dot)nenciarini(at)2ndquadrant(dot)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(at)gmail(dot)com> writes:
> On Wed, Jul 11, 2012 at 9:54 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> wrote:
> On Wed, Jul 11, 2012 at 9:54 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Marco Nenciarini <marco(dot)nenciarini(at)2ndquadrant(dot)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(dot)nenciarini(at)2ndquadrant(dot)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