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