On Thu, 09 Nov 2017 12:26:46 +0000
Aleksander Alekseev <a.alekseev@postgrespro.ru> wrote:
> The following review has been posted through the commitfest
> application: make installcheck-world: tested, failed
> Implements feature: tested, passed
> Spec compliant: tested, passed
> Documentation: tested, passed
>
> Hello Anthony,
>
> Great job!
>
> I decided to take a closer look on your patch. Here are some defects
> I discovered.
>
> > + Additional extensions are available that implement transforms
> > for
> > + the <type>jsonb</type> type for the language PL/Python. The
> > + extensions for PL/Perl are called
>
> 1. The part regarding PL/Perl is obviously from another patch.
>
> 2. jsonb_plpython2u and jsonb_plpythonu are marked as relocatable,
> while jsonb_plpython3u is not. Is it a mistake? Anyway if an
> extension is relocatable there should be a test that checks this.
>
> 3. Not all json types are test-covered. Tests for 'true' :: jsonb,
> '3.14' :: jsonb and 'null' :: jsonb are missing.
>
> 4. jsonb_plpython.c:133 - "Iterate trhrough Jsonb object." Typo, it
> should be "through" or probably even "over".
>
> 5. It looks like you've implemented transform in two directions
> Python <-> JSONB, however I see tests only for Python <- JSONB case.
>
> 6. Tests passed on Python 2.7.14 but failed on 3.6.2:
>
> > CREATE EXTENSION jsonb_plpython3u CASCADE;
> > + ERROR: could not access file "$libdir/jsonb_plpython3": No such
> > file or directory
>
> module_pathname in jsonb_plpython3u.control should be
> $libdir/jsonb_plpython3u, not $libdir/jsonb_plpython3.
>
> Tested on Arch Linux x64, GCC 7.2.0.
>
> The new status of this patch is: Waiting on Author
>
Hello, Aleksander.
Thank you for your time. The defects you have noticed were fixed.
Please, find in attachments new version of the patch (it is called
0001-jsonb_plpython-extension-v2.patch).
Most of changes were made to fix defects(list of the defects may be
found in citation in the beginning of this message), but the algorithm
of iterating through incoming jsonb was changed so that it looks tidier.
--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
