Date: Mon, 20 Nov 2017 16:48:36 +0300
From: Aleksander Alekseev <a.alekseev@postgrespro.ru>
To: Anthony Bykov <a.bykov@postgrespro.ru>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Jsonb transform for pl/python
Message-ID: <20171120134835.GA9855@e733.localdomain>
References: <20171025145100.3a19933f@anthony-24-g082ur>
 <20171113130028.32405fee@anthony-24-g082ur>
 <20171113150816.1377.17052.pgcf@coridan.postgresql.org>
 <20171120123555.546c922f@anthony-24-g082ur>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="VbJkn9YxBvnuCH5J"
Content-Disposition: inline
In-Reply-To: <20171120123555.546c922f@anthony-24-g082ur>
User-Agent: Mutt/1.9.1 (2017-09-22)
Precedence: bulk


--VbJkn9YxBvnuCH5J
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.=20

Well frankly I very much doubt that this:

```
+-- set -> jsonb
+CREATE FUNCTION test1set() RETURNS jsonb
+LANGUAGE plpython2u
+TRANSFORM FOR TYPE jsonb
+AS $$
+x =3D set()
+x.add(1)
+x.add("string")
+x.add(None)
+return x
+$$;
+SELECT test1set();
+          test1set         =20
+----------------------------
+ "set([1, 'string', None])"
+(1 row)
```

=2E.. 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 =3D 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.
>=20
> 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       =20
  -----------------------
!  "{None, 1, 'string'}"
  (1 row)
 =20
  DROP EXTENSION plpython3u CASCADE;
--- 296,302 ----
  SELECT test1set();
         test1set       =20
  -----------------------
!  "{1, None, 'string'}"
  (1 row)
 =20
  DROP EXTENSION plpython3u CASCADE
```

--=20
Best regards,
Aleksander Alekseev

--VbJkn9YxBvnuCH5J
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEECAokn0QZCoIHW4GpEEnCBLXCDtYFAloS3TEACgkQEEnCBLXC
DtZh+A/+PjDh4ZiDXpyQgMfstIr2mbsXJ5kmp1ZpNoAZVdffO2dfg95W3QL2GEK6
qBL7aLHd40Inx5nU0wDI3ydpka/GFODhHMzwfqq4GkCt0GmJsdhvyRER8vCibBZy
BfnDyHa0KC77FdNilfm9vH3AmhBrkutiJFv6z55UpIqUJA8P9rAwrzAss6VXR5Id
9HM2jaWgP99MT0ih8PMOq7uM8ii5GqpHhERYUUTTeZaQ7NkPfMqwiLmKbAJKrUfO
tP4sH4feaETb9RGNEifDBEin4e03uddnDkiN7mZvF5nVAlx5xTsQFxDfY8BMdci+
0E4fbNJ3XxaIZm5UOXOIPwUXtNVlNH3wMXIXJAik6TXe1pperLO+tDX1khqTpwzr
53T7nOMAXLurBl7PmSQARuhE56KYdm2goh9Jey5vTRzkW5mi/i08mzsIaIfac7q1
fS0haqk4Z4tochghRtFIWn0fSmO/+vQGvx98T6YXW2AETo2kPkXPzuaUHWImXUj4
hyhXmQCnUZrhXLaNP/lnUvwBMyzqg9fv2nRE71xd4hZKzbfV4jUmkjrPnogs6Rmy
UYOWMDhV7xqaLNLDZuAQg2ePHz4t0TIm+8wBi14j3dYRk1RluRJNW0PCgDvrFpfS
pnXTY2IJRwpVu5tMGaKecyW83YLRJ1zzHObCLqLsF/vvfMY6CA4=
=Faoc
-----END PGP SIGNATURE-----

--VbJkn9YxBvnuCH5J--