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
