Thread: 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,
RodolfoAttachment
On Sat, Oct 26, 2013 at 9:17 AM, Rodolfo Campero <rodolfo.campero@anachronics.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
<div dir="ltr">Done, thanks.<div class="gmail_extra"><br /><br /><div class="gmail_quote">2013/10/28 Robert Haas <span dir="ltr"><<ahref="mailto:robertmhaas@gmail.com" target="_blank">robertmhaas@gmail.com</a>></span><br /><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><divclass="h5">On Sat, Oct 26, 2013 at 9:17 AM, Rodolfo Campero<br /> <<a href="mailto:rodolfo.campero@anachronics.com">rodolfo.campero@anachronics.com</a>>wrote:<br /> > The attached patchadd support of domains over arrays to PL/Python (eg:<br /> > CREATE DOMAIN my_domain AS integer[]).<br /> ><br/> > Basically it just uses get_base_element_type instead of get_element_type in<br /> > plpy_typeio.c, anduses domain_check before returning a sequence as array in<br /> > PLySequence_ToArray whenever appropriate.<br /> ><br/> > There's one line I'm not sure about; I modified a switch statement (line<br /> > 427):<br /> > switch(element_type ? element_type : getBaseType(arg->typoid))<br /> > The rationale is that when element_type is set,it is already a base type,<br /> > because there is no support of arrays of domains in PostgreSQL, but this may<br/> > not held true in the future.<br /><br /></div></div>Please add your patch here so that it doesn't get forgottenabout:<br /><br /><a href="https://commitfest.postgresql.org/action/commitfest_view/open" target="_blank">https://commitfest.postgresql.org/action/commitfest_view/open</a><br/><span class="HOEnZb"><font color="#888888"><br/> --<br /> Robert Haas<br /> EnterpriseDB: <a href="http://www.enterprisedb.com" target="_blank">http://www.enterprisedb.com</a><br/> The Enterprise PostgreSQL Company<br /></font></span></blockquote></div><br/></div></div>
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@gmail.com>
There was no actual need to modify that, so I dropped that change in this new patch.
Thanks for your review.
On Sat, Oct 26, 2013 at 11:17:19AM -0200, Rodolfo Campero wrote:Generally looks fine. Please lose the C++ comments though, this style
> 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.
is not used in Postgres sources.
Done.
Was there any actual need to modify that? Or was it just performance
> 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.
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.
Attachment
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@gmail.com>
The attached patch (version 3) includes the expected results for Python 3 (file plpython_types_3.out).
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).
Attachment
On Sat, Nov 23, 2013 at 11:09:53AM -0200, Rodolfo Campero wrote: > 2013/11/22 Marko Kreen <markokr@gmail.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@gmail.com>
Thanks. Looks good now.On Sat, Nov 23, 2013 at 11:09:53AM -0200, Rodolfo Campero wrote:
> 2013/11/22 Marko Kreen <markokr@gmail.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).
Tagging as ready for committer.
--
marko
Rodolfo Campero
Anachronics S.R.L.
Tel: (54 11) 4899 2088
rodolfo.campero@anachronics.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@gmail.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@vmware.com>
[...]
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.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
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
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@vmware.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@vmware.com> wrote: > >> Ok, committed. > > make check-world failure: Oops, sorry about that. Fixed. - Heikki
2013/11/26 Heikki Linnakangas <hlinnakangas@vmware.com>
--
Rodolfo
On 11/26/13 19:07, Kevin Grittner wrote:Oops, sorry about that. Fixed.Heikki Linnakangas <hlinnakangas@vmware.com> wrote:Ok, committed.
make check-world failure:
Maybe be you forgot to modify plpython_types_3.out?
--
Rodolfo
On Tue, Nov 26, 2013 at 07:12:00PM -0200, Rodolfo Campero wrote: > 2013/11/26 Heikki Linnakangas <hlinnakangas@vmware.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
Attachment
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@vmware.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