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
>
>
>