Re: [PATH] Jsonb, insert a new value into an array at arbitrary position
От | Vitaly Burovoy |
---|---|
Тема | Re: [PATH] Jsonb, insert a new value into an array at arbitrary position |
Дата | |
Msg-id | CAKOSWNnpk2v8+e=kx5f0OfB2v+_wJDxv6tXvc5fro5D4T3p1Mg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [PATH] Jsonb, insert a new value into an array at arbitrary position (Dmitry Dolgov <9erthalion6@gmail.com>) |
Ответы |
Re: [PATH] Jsonb, insert a new value into an array at arbitrary
position
(David Steele <david@pgmasters.net>)
|
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления: