On 2016-02-29 17:19+00, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
On 2016-02-24 19:37+00, Petr Jelinek <petr@2ndquadrant.com> wrote:
>> Also this patch needs documentation.
> I've added new version in attachments, thanks.
Hello! The first pass of a review is below.
1. Rename the "flag" variable to something more meaningful. (e.g.
"op_type" - "operation_type")
2. There is two extra spaces after the "_DELETE" word
+#define JB_PATH_DELETE 0x0002
3. res = setPath(&it, path_elems, path_nulls, path_len, &st,
- 0, newval, create);
+ 0, newval, create ? JB_PATH_CREATE : 0x0);
What is the magic constant "0x0"? If not "create", then what?
Introduce something like JB_PATH_NOOP = 0x0...
4. In the "setPathArray" the block: if (newval != NULL)
"newval == NULL" is considered as "to be deleted" from the previous
convention and from the comments for the "setPath" function.
I think since you've introduced special constants one of which is
JB_PATH_DELETE (which is not used now) you should replace convention
from checking for a value to checking for a constant: if (flag != JB_PATH_DELETE)
or even better: if (!flag & JB_PATH_DELETE)
5. Change checking for equality (to bit constants) to bit operations: (flag == JB_PATH_INSERT_BEFORE || flag ==
JB_PATH_INSERT_AFTER)
which can be replaced to: (flag & (JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER))
also here: (flag == JB_PATH_CREATE || flag == JB_PATH_INSERT_BEFORE || flag
== JB_PATH_INSERT_AFTER))
can be: (flag & (JB_PATH_CREATE | JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER))
6. Pay attention to parenthesises to make the code looks like similar
one around:
+ if ((npairs == 0) && flag == JB_PATH_CREATE && (level == path_len - 1))
should be:
+ if ((npairs == 0) && (flag == JB_PATH_CREATE) && (level == path_len - 1))
7. Why did you remove "skip"? It is a comment what "true" means...
- r = JsonbIteratorNext(it, &v, true); /* skip */
+ r = JsonbIteratorNext(it, &v, true);
8. After your changes some statements exceed 80-column threshold...
The same rules for the documentation.
9. And finally... it does not work as expected in case of:
postgres=# select jsonb_insert('{"a":[0,1,2,3]}', '{"a", 10}', '"4"'); jsonb_insert
---------------------{"a": [0, 1, 2, 3]}
(1 row)
wheras jsonb_set works:
postgres=# select jsonb_set('{"a":[0,1,2,3]}', '{"a", 10}', '"4"'); jsonb_set
--------------------------{"a": [0, 1, 2, 3, "4"]}
(1 row)
--
Best regards,
Vitaly Burovoy