Re: Jsonb transform for pl/python - Mailing list pgsql-hackers

On Mon, 13 Nov 2017 15:08:16 +0000
Aleksander Alekseev <a.alekseev@postgrespro.ru> wrote:

> The following review has been posted through the commitfest
> application: make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed
> 
> Hi Anthony,
> 
> Thank you for the new version of the patch! Here is my code review.
> 
> 1. In jsonb_plpython2.out:
> 
> +CREATE FUNCTION back(val jsonb) RETURNS jsonb
> +LANGUAGE plpython2u
> +TRANSFORM FOR TYPE jsonb
> +as $$
> +return val
> +$$;
> +SELECT back('null'::jsonb);
> +  back  
> +--------
> + [null]
> +(1 row)
> +
> +SELECT back('1'::jsonb);
> + back 
> +------
> + [1]
> +(1 row)
> +
> +SELECT back('true'::jsonb);
> +  back  
> +--------
> + [true]
> +(1 row)
> 
> Maybe I'm missing something, but why exactly all JSONB values turn
> into arrays?
> 
> 2. Could you please also add tests for some negative and real
> numbers? Also could you check that your code handles numbers like
> MAX_INT, MIN_INT, +/- infinity and NaN properly in both (Python <->
> JSONB) directions?
> 
> 3. Handling unicode strings properly is another thing that is worth
> checking.
> 
> 4. I think we also need some tests that check the behavior of Python
> -> JSONB conversion when the object contains data that is not
> representable in JSON format, e.g. set(), some custom objects, etc. 
> 
> 5. PyObject_FromJsonbValue - I realize it's unlikely that the new
> jsonbValue->type will be introduced any time soon. Still I believe
> it's a good practice to add "it should never happen" default case
> that just does elog(ERROR, ...) in case it happens nevertheless.
> Otherwise in this scenario instead of reporting the error the code
> will silently do the wrong thing.
> 
> 6. Well, you decided to make the extension non-relocatable. Could you
> at least describe what prevents it to be relocatable or why it's
> meaningless is a comment in .control file? Please note that almost
> all contrib/ extensions are relocatable. I believe your extension
> should be relocatable as well unless there is a good reason why it
> can't.
> 
> The new status of this patch is: Waiting on Author

Hi,
thank you for your review. I took your comments into account in the
third version of the patch. In the new version, I've added all the
tests you asked for. The interesting thing is that:
1. set or any other non-jsonb-transformable object is transformed into
string and added to jsonb as a string. 
2. couldn't find a solution of working with "inf": this extension
troughs exception if python generates a number called "inf" and returns
it, but if we pass a very large integer into a function, it works fine
with the whole number. This situation can be seen in tests.

I've added tests of large numerics which weights quite heavy. So,
please find it in compressed format in attachments.


--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company