PL/Python: domain over array support

The attached patch add support of domains over arrays to PL/Python (eg:
CREATE DOMAIN my_domain AS integer[]).

Basically it just uses get_base_element_type instead of get_element_type
in plpy_typeio.c, and uses domain_check before returning a sequence as
array in PLySequence_ToArray whenever appropriate.

There's one line I'm not sure about; I modified a switch statement (line
427):
switch (element_type ? element_type : getBaseType(arg->typoid))
The rationale is that when element_type is set, it is already a base type,
because there is no support of arrays of domains in PostgreSQL, but this
may not held true in the future.

Regards,
Rodolfo

On Sat, Oct 26, 2013 at 9:17 AM, Rodolfo Campero
<rodolfo(dot)campero(at)anachronics(dot)com> wrote:
> The attached patch add support of domains over arrays to PL/Python (eg:
> CREATE DOMAIN my_domain AS integer[]).
>
> Basically it just uses get_base_element_type instead of get_element_type in
> plpy_typeio.c, and uses domain_check before returning a sequence as array in
> PLySequence_ToArray whenever appropriate.
>
> There's one line I'm not sure about; I modified a switch statement (line
> 427):
> switch (element_type ? element_type : getBaseType(arg->typoid))
> The rationale is that when element_type is set, it is already a base type,
> because there is no support of arrays of domains in PostgreSQL, but this may
> not held true in the future.

Please add your patch here so that it doesn't get forgotten about:

https://commitfest.postgresql.org/action/commitfest_view/open

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Done, thanks.

2013/10/28 Robert Haas <robertmhaas(at)gmail(dot)com>

> On Sat, Oct 26, 2013 at 9:17 AM, Rodolfo Campero
> <rodolfo(dot)campero(at)anachronics(dot)com> wrote:
> > The attached patch add support of domains over arrays to PL/Python (eg:
> > CREATE DOMAIN my_domain AS integer[]).
> >
> > Basically it just uses get_base_element_type instead of get_element_type
> in
> > plpy_typeio.c, and uses domain_check before returning a sequence as
> array in
> > PLySequence_ToArray whenever appropriate.
> >
> > There's one line I'm not sure about; I modified a switch statement (line
> > 427):
> > switch (element_type ? element_type : getBaseType(arg->typoid))
> > The rationale is that when element_type is set, it is already a base
> type,
> > because there is no support of arrays of domains in PostgreSQL, but this
> may
> > not held true in the future.
>
> Please add your patch here so that it doesn't get forgotten about:
>
> https://commitfest.postgresql.org/action/commitfest_view/open
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

On Sat, Oct 26, 2013 at 11:17:19AM -0200, Rodolfo Campero wrote:
> The attached patch add support of domains over arrays to PL/Python (eg:
> CREATE DOMAIN my_domain AS integer[]).
>
> Basically it just uses get_base_element_type instead of get_element_type
> in plpy_typeio.c, and uses domain_check before returning a sequence as
> array in PLySequence_ToArray whenever appropriate.

Generally looks fine. Please lose the C++ comments though, this style
is not used in Postgres sources.

> There's one line I'm not sure about; I modified a switch statement (line
> 427):
> switch (element_type ? element_type : getBaseType(arg->typoid))
> The rationale is that when element_type is set, it is already a base type,
> because there is no support of arrays of domains in PostgreSQL, but this
> may not held true in the future.

Was there any actual need to modify that? Or was it just performance
optimization? ATM it creates asymmetry between PLy_output_datum_func2
and PLy_input_datum_func2.

If it's just performace optimization, then it should be done in both
functions, but seems bad idea to do it in this patch. So I think
it's better to leave it out.

--
marko

Marko,

2013/11/22 Marko Kreen <markokr(at)gmail(dot)com>

> On Sat, Oct 26, 2013 at 11:17:19AM -0200, Rodolfo Campero wrote:
> > The attached patch add support of domains over arrays to PL/Python (eg:
> > CREATE DOMAIN my_domain AS integer[]).
> >
> > Basically it just uses get_base_element_type instead of get_element_type
> > in plpy_typeio.c, and uses domain_check before returning a sequence as
> > array in PLySequence_ToArray whenever appropriate.
>
> Generally looks fine. Please lose the C++ comments though, this style
> is not used in Postgres sources.
>

Done.

> > There's one line I'm not sure about; I modified a switch statement (line
> > 427):
> > switch (element_type ? element_type : getBaseType(arg->typoid))
> > The rationale is that when element_type is set, it is already a base
> type,
> > because there is no support of arrays of domains in PostgreSQL, but this
> > may not held true in the future.
>
> Was there any actual need to modify that? Or was it just performance
> optimization? ATM it creates asymmetry between PLy_output_datum_func2
> and PLy_input_datum_func2.
>
> If it's just performace optimization, then it should be done in both
> functions, but seems bad idea to do it in this patch. So I think
> it's better to leave it out.
>
>
There was no actual need to modify that, so I dropped that change in this
new patch.

There are other cosmetic changes in this patch, wrt previous version (not
preexistent code):
* adjusted alignment of variable name "rv" in line 12
* reworded comment in line 850, resulting in more than 80 characters, so I
splitted the comment into a multiline comment following the surrounding
style.

Thanks for your review.

On Fri, Nov 22, 2013 at 08:45:56AM -0200, Rodolfo Campero wrote:
> There are other cosmetic changes in this patch, wrt previous version (not
> preexistent code):
> * adjusted alignment of variable name "rv" in line 12
> * reworded comment in line 850, resulting in more than 80 characters, so I
> splitted the comment into a multiline comment following the surrounding
> style.

Good.

One more thing - please update Python 3 regtests too.

--
marko

2013/11/22 Marko Kreen <markokr(at)gmail(dot)com>

>
> One more thing - please update Python 3 regtests too.
>
>
The attached patch (version 3) includes the expected results for Python 3
(file plpython_types_3.out).

On Sat, Nov 23, 2013 at 11:09:53AM -0200, Rodolfo Campero wrote:
> 2013/11/22 Marko Kreen <markokr(at)gmail(dot)com>
> > One more thing - please update Python 3 regtests too.
> >
> The attached patch (version 3) includes the expected results for Python 3
> (file plpython_types_3.out).

Thanks. Looks good now.

Tagging as ready for committer.

--
marko

Thank you very much Marko.

2013/11/24 Marko Kreen <markokr(at)gmail(dot)com>

> On Sat, Nov 23, 2013 at 11:09:53AM -0200, Rodolfo Campero wrote:
> > 2013/11/22 Marko Kreen <markokr(at)gmail(dot)com>
> > > One more thing - please update Python 3 regtests too.
> > >
> > The attached patch (version 3) includes the expected results for Python 3
> > (file plpython_types_3.out).
>
> Thanks. Looks good now.
>
> Tagging as ready for committer.
>
> --
> marko
>
>

--
Rodolfo Campero
Anachronics S.R.L.
Tel: (54 11) 4899 2088
rodolfo(dot)campero(at)anachronics(dot)com
http://www.anachronics.com

On 24.11.2013 18:44, Marko Kreen wrote:
> On Sat, Nov 23, 2013 at 11:09:53AM -0200, Rodolfo Campero wrote:
>> 2013/11/22 Marko Kreen <markokr(at)gmail(dot)com>
>>> One more thing - please update Python 3 regtests too.
>>>
>> The attached patch (version 3) includes the expected results for Python 3
>> (file plpython_types_3.out).
>
> Thanks. Looks good now.

Looks good to me too.

This does change the behavior of any existing functions that return a
domain over array. For example:

postgres=# create domain intarr as integer[];
CREATE DOMAIN
postgres=# create function intarr_test() returns intarr as $$
return '{1,2}'
$$ language plpythonu;
CREATE FUNCTION

Before patch:

postgres=# select intarr_test();
intarr_test
-------------
{1,2}
(1 row)

After patch:

postgres=# select intarr_test();
ERROR: invalid input syntax for integer: "{"
CONTEXT: while creating return value
PL/Python function "intarr_test"

The new behavior is clearly better, but it is an incompatibility
nonetheless. I don't do anything with PL/python myself, so I don't have
a good feel of how much that'll break people's applications. Probably
not much I guess. But warrants a mention in the release notes at least.
Any thoughts on that?

- Heikki

2013/11/25 Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>
[...]

> This does change the behavior of any existing functions that return a
> domain over array. For example:
>
> postgres=# create domain intarr as integer[];
> CREATE DOMAIN
> postgres=# create function intarr_test() returns intarr as $$
> return '{1,2}'
> $$ language plpythonu;
> CREATE FUNCTION
>
> Before patch:
>
> postgres=# select intarr_test();
> intarr_test
> -------------
> {1,2}
> (1 row)
>
> After patch:
>
> postgres=# select intarr_test();
> ERROR: invalid input syntax for integer: "{"
> CONTEXT: while creating return value
> PL/Python function "intarr_test"
>
>
> The new behavior is clearly better, but it is an incompatibility
> nonetheless. I don't do anything with PL/python myself, so I don't have a
> good feel of how much that'll break people's applications. Probably not
> much I guess. But warrants a mention in the release notes at least. Any
> thoughts on that?
>
> - Heikki
>

Bear in mind that the same goes for receiving domains over arrays as
parameters; instead of seeing a string (previous behavior), with this patch
a function will see a list from the Python side (the function
implementation). A mention in the release notes is in order, I agree with
that.

I can't speak for other people, but I guess using domains over arrays as
parameters and/or return values in plpythonu functions should be rare,
considering the current behavior and especially given the possibility of
using a regular array in order to handle array values a lists in Python.

Regards,
--
Rodolfo

On Tue, Nov 26, 2013 at 12:23:48AM +0200, Heikki Linnakangas wrote:
> The new behavior is clearly better, but it is an incompatibility
> nonetheless. I don't do anything with PL/python myself, so I don't
> have a good feel of how much that'll break people's applications.
> Probably not much I guess. But warrants a mention in the release
> notes at least. Any thoughts on that?

Yes it warrants a mention but nothing excessive, in 9.0 the non-domain
arrays has same non-compatible change with only one sentence in notes.

--
marko

On 11/26/13 11:56, Marko Kreen wrote:
> On Tue, Nov 26, 2013 at 12:23:48AM +0200, Heikki Linnakangas wrote:
>> The new behavior is clearly better, but it is an incompatibility
>> nonetheless. I don't do anything with PL/python myself, so I don't
>> have a good feel of how much that'll break people's applications.
>> Probably not much I guess. But warrants a mention in the release
>> notes at least. Any thoughts on that?
>
> Yes it warrants a mention but nothing excessive, in 9.0 the non-domain
> arrays has same non-compatible change with only one sentence in notes.

Ok, committed. Thank you, Rodolfo and Marko!

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:

> Ok, committed.

make check-world failure:

*** /home/kgrittn/pg/master/src/pl/plpython/expected/plpython_types.out 2013-11-26 10:52:04.173441894 -0600
--- /home/kgrittn/pg/master/src/pl/plpython/results/plpython_types.out  2013-11-26 10:55:58.229445970 -0600
***************
*** 664,669 ****
--- 664,672 ----
  ERROR:  return value of function with array return type is not a Python sequence
  CONTEXT:  while creating return value
  PL/Python function "test_type_conversion_array_error"
+ --
+ -- Domains over arrays
+ --
  CREATE DOMAIN ordered_pair_domain AS integer[] CHECK (array_length(VALUE,1)=2 AND VALUE[1] < VALUE[2]);
  CREATE FUNCTION test_type_conversion_array_domain(x ordered_pair_domain) RETURNS ordered_pair_domain AS $$
  plpy.info(x, type(x))

======================================================================

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/26/13 19:07, Kevin Grittner wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:
>
>> Ok, committed.
>
> make check-world failure:

Oops, sorry about that. Fixed.

- Heikki

2013/11/26 Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>

> On 11/26/13 19:07, Kevin Grittner wrote:
>
>> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:
>>
>> Ok, committed.
>>>
>>
>> make check-world failure:
>>
>
> Oops, sorry about that. Fixed.

Maybe be you forgot to modify
plpython_types_3.out<http://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/pl/plpython/expected/plpython_types_3.out;h=25331f268a1e456a484ccba328198eed2cd87313;hb=37364c63115a52b4dc7ea280aa5b358abd4a9c38>
?

--
Rodolfo

On Tue, Nov 26, 2013 at 07:12:00PM -0200, Rodolfo Campero wrote:
> 2013/11/26 Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>
> > Oops, sorry about that. Fixed.
>
> Maybe be you forgot to modify
> plpython_types_3.out

Yes. Heikki, please fix plpython_types_3.out too.

See attached patch.

--
marko

On 11/27/13 14:15, Marko Kreen wrote:
> On Tue, Nov 26, 2013 at 07:12:00PM -0200, Rodolfo Campero wrote:
>> 2013/11/26 Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>
>>> Oops, sorry about that. Fixed.
>>
>> Maybe be you forgot to modify
>> plpython_types_3.out
>
> Yes. Heikki, please fix plpython_types_3.out too.
>
> See attached patch.

Ah, sorry. Committed..

- Heikki