Horton
Menu

Thread

  1. Support for array_remove and array_replace functions

    Marco Nenciarini <marco.nenciarini@2ndquadrant.it> — 2012-06-14T10:41:53Z

    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

  2. Re: Support for array_remove and array_replace functions

    Alex Hunsaker <badalex@gmail.com> — 2012-06-30T02:16:08Z

    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.

  3. Re: Support for array_remove and array_replace functions

    Marco Nenciarini <marco.nenciarini@2ndquadrant.it> — 2012-06-30T21:28:28Z

    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

  4. Re: Support for array_remove and array_replace functions

    Alex Hunsaker <badalex@gmail.com> — 2012-07-01T19:20:58Z

    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.

  5. Re: Support for array_remove and array_replace functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-07-11T15:54:50Z

    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

  6. Re: Support for array_remove and array_replace functions

    Alex Hunsaker <badalex@gmail.com> — 2012-07-11T16:53:52Z

    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.

  7. Re: Support for array_remove and array_replace functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-07-11T17:00:34Z

    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

  8. Re: Support for array_remove and array_replace functions

    Robert Haas <robertmhaas@gmail.com> — 2012-07-11T17:40:50Z

    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

  9. Re: Support for array_remove and array_replace functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-07-11T18:06:55Z

    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