Re: [PATCH] Fix conversion for Decimal arguments in plpython functions

On 05/28/2013 04:41 PM, Szymon Guz wrote:
> Hi,
> I've got a patch.
>
> This is for a plpython enhancement.
>
> There is an item at the TODO list
> http://wiki.postgresql.org/wiki/Todo#Server-Side_Languages
> "Fix loss of information during conversion of numeric type to Python
> float"
>
> This patch uses a decimal.Decimal type from Python standard library
> for the plpthon function numeric argument instead of float.
>
> Patch contains changes in code, documentation and tests.
>
> Most probably there is something wrong, as this is my first Postgres
> patch :)
>

Thanks for contributing.

This patch applies cleanly against master and compiles with warnings

plpy_main.c: In function ‘PLy_init_interp’:
plpy_main.c:157:2: warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
plpy_main.c:161:2: warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]

You can avoid this by moving the declaration of decimal and decimal_dict
to be at the top of the function where mainmod is declared.

Also in this function you've introduced places where it returns with an
error (the PLy_elog(ERROR...) calls before decrementing the reference to
mainmod. I think you can decrement the mainmod reference after the call
to SetItemString before your changes that import the Decimal module.

The patch works as expected, I am able to write python functions that
take numerics as arguments and work with them. I can adjust the decimal
context precision inside of my function.

One concern I have is that this patch makes pl/python functions
involving numerics more than 3 times as slow as before.

create temp table b(a numeric);
insert into b select generate_series(1,10000);

create or replace function x(a numeric,b numeric) returns numeric as $$
if a==None:
return b
return a+b
$$ language plpythonu;
create aggregate sm(basetype=numeric, sfunc=x,stype=numeric);

test=# select sm(a) from b;
sm
----------
50005000
(1 row)

Time: 565.650 ms

versus before the patch this was taking in the range of 80ms.

Would it be faster to call numeric_send instead of numeric_out and then
convert the sequence of Int16's to a tuple of digits that can be passed
into the Decimal constructor? I think this is worth trying and testing,

Documentation
=================
Your patched version of the docs say

PostgreSQL <type>real</type>, <type>double</type>, and
<type>numeric</type> are converted to
Python <type>Decimal</type>. This type is imported
from<literal>decimal.Decimal</literal>.

I don't think this is correct, as far as I can tell your not changing
the behaviour for postgresql real and double types, they continue to use
floating point.

<listitem>
<para>
PostgreSQL <type>real</type> and <type>double</type>are converted to
Python <type>float</type>.
</para>
</listitem>

<listitem>
<para>
PostgreSQL <type>numeric</type> is converted to
Python <type>Decimal</type>. This type is imported from
<literal>decimal.Decimal</literal>.
</para>
</listitem>

Maybe?

Steve

> thanks,
> Szymon
>
>
>