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

On 27 June 2013 05:21, Steve Singer <steve(at)ssinger(dot)info> wrote:

> On 06/26/2013 04:47 PM, Szymon Guz wrote:
>
>>
>>
>>
>>
>> Attached patch has all changes against trunk code.
>>
>> There is added a function for conversion from Postgres numeric to Python
>> Decimal. The Decimal type is taken from cdecimal.Decimal, if it is
>> available. It is an external library, quite fast, but may be not available.
>> If it is not available, then decimal.Decimal will be used. It is in
>> standard Python library, however it is rather slow.
>>
>> The initialization is done in the conversion function, the pointer to a
>> proper Decimal constructor is stored as static variable inside the function
>> and is lazy initialized.
>>
>> The documentation is updated.
>>
>>
> I've tested this version with python 2.7 with and without cdecimal and
> also with 3.3 that has the faster decimal performance. It seems fine.
>
> The v5 version of the patch makes only white-space changes to plpy_main.c
> you should excluded that from the patch if your making a new version (I
> have done this in the v6 version I'm attaching)
>
>
>
> Tests for python 2 and 3 have been added. They work only with standard
>> decimal.Decimal, as the type is printed in the *.out files. I think there
>> is nothing we can do with that now.
>>
>>
>>
> I think we should make test_type_conversion_numeric to do something that
> generates the same output in both cases. ie
> py.info(str(x)). I downside of having the test fail on installs with
> cdecimal installed is much greater than any benefit we get by ensuring that
> the type is really decimal.
> I've attached a v6 version of the patch that does this, do you agree with
> my thinking?
>
>
Hi Steve,
thanks for the changes.

You're idea about common code for decimal and cdecimal is good, however not
good enough. I like the idea of common code for decimal and cdecimal. But
we need class name, not the value.

I've changed the code from str(x) to x.__class__.__name__ so the function
prints class name (which is Decimal for both packages), not the value. We
need to have the class name check. The value is returned by the function
and is a couple of lines lower in the file.

patch is attached.

thanks,
Szymon