Обсуждение: 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.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it

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