From: | Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru> |
---|---|
To: | Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru> |
Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Jsonb transform for pl/python |
Date: | 2017-11-20 13:48:36 |
Message-ID: | 20171120134835.GA9855@e733.localdomain |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi Anthony,
> 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.
Well frankly I very much doubt that this:
```
+-- set -> jsonb
+CREATE FUNCTION test1set() RETURNS jsonb
+LANGUAGE plpython2u
+TRANSFORM FOR TYPE jsonb
+AS $$
+x = set()
+x.add(1)
+x.add("string")
+x.add(None)
+return x
+$$;
+SELECT test1set();
+ test1set
+----------------------------
+ "set([1, 'string', None])"
+(1 row)
```
... is an expected and valid behavior. If user tries to transform a
set() to JSONB this is most likely a mistake since there is no standard
representation of a set() in JSONB. I believe we should rise an error in
this case instead of generating a string. Besides user can expect that
such string can be transformed back to set() which doesn't sound like a
good idea either.
If necessary, user can just transform a set() to a list():
```
>>> x = set([1,2,3,4])
>>> x
{1, 2, 3, 4}
>>> list(x)
[1, 2, 3, 4]
```
BTW I just recalled that Python supports complex numbers out-of-the box
and that range and xrange are a separate types too:
```
>>> 1 + 2j
(1+2j)
>>> range(3)
range(0, 3)
>>> type(range(3))
<class 'range'>
```
I think we should add all this to the tests as well. Naturally complex
numbers can't be represented in JSON so we should rise an error if user
tries to transform a complex number to JSON. I'm not that sure regarding
ranges though. Probably the best solution will be to rise and error in
this case as well just to keep things consistent.
> +ERROR: jsonb doesn't support inf type yet
I would say this error message is too informal. How about something more
like "Infinity can't be represented in JSONB"?
> 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.
I'm afraid that tests fail on Python 3:
```
SELECT test1set();
test1set
-----------------------
! "{None, 1, 'string'}"
(1 row)
DROP EXTENSION plpython3u CASCADE;
--- 296,302 ----
SELECT test1set();
test1set
-----------------------
! "{1, None, 'string'}"
(1 row)
DROP EXTENSION plpython3u CASCADE
```
--
Best regards,
Aleksander Alekseev
From | Date | Subject | |
---|---|---|---|
Next Message | Alik Khilazhev | 2017-11-20 15:16:59 | Re: [HACKERS] [WIP] Zipfian distribution in pgbench |
Previous Message | Amit Kapila | 2017-11-20 13:41:50 | Re: [HACKERS] A GUC to prevent leader processes from running subplans? |