Re: mogrify and indent features for jsonb - Mailing list pgsql-hackers
| From | Dmitry Dolgov |
|---|---|
| Subject | Re: mogrify and indent features for jsonb |
| Date | |
| Msg-id | CA+q6zcVH6YDmTZh-EepFvDQ62Ez0T1gLpiRDXDBQ72=O4Lp01w@mail.gmail.com Whole thread Raw |
| In response to | Re: mogrify and indent features for jsonb (Petr Jelinek <petr@2ndquadrant.com>) |
| Responses |
Re: mogrify and indent features for jsonb
(Petr Jelinek <petr@2ndquadrant.com>)
|
| List | pgsql-hackers |
Hi, Petr, thanks for the review.
>>> I think it would be better if the ident printing didn't put the start of array ([) and start of dictionary ({) on separate line
Did you mean this?
[
{
"a": 1,
"b": 2
}
]
I tried to verify this in several ways (http://jsonprettyprint.com/, "JSON.stringify", "json.too" from python), and I always get this result (new line after ([) and ({) ).
>>> I don't really understand the point of h_atoi() function.
Initially, this function was to combine the convertion logic and specific verifications. But I agree, "strtol" is more correct way, I should improve this.
>>> The code looks ok as well except maybe the replacePath could use couple of comments
I already added several commentaries (and looks like I should add even more in the nearest future) for this function in the jsonbx extension, and I think we can update this patch one more time with that improvement.
>>> About the Assert(ARR_ELEMTYPE(path) == TEXTOID);
I based my work on the hstore extension, which contains such kind of assertions. But I suppose, it's not required anymore, so I removed this from the extension. And, I think, we can also remove this from patch.
On 18 February 2015 at 08:32, Petr Jelinek <petr@2ndquadrant.com> wrote:
Hi,
I looked at the patch and have several comments.
First let me say that modifying the individual paths of the json is the feature I miss the most in the current implementation so I am happy that this patch was submitted.
I would prefer slightly the patch split into two parts, one for the indent printing and one with the manipulation functions, but it's not too big patch so it's not too bad as it is.
There is one compiler warning that I see:
jsonfuncs.c:3533:1: warning: no previous prototype for ‘jsonb_delete_path’ [-Wmissing-prototypes]
jsonb_delete_path(PG_FUNCTION_ARGS)
I think it would be better if the ident printing didn't put the start of array ([) and start of dictionary ({) on separate line since most "pretty" printing implementations outside of Postgres (like the JSON.stringify or python's json module) don't do that. This is purely about consistency with the rest of the world.
The json_ident might be better named as json_pretty perhaps?
I don't really understand the point of h_atoi() function. What's wrong with using strtol like pg_atoi does? Also there is no integer overflow check anywhere in that function.
There is tons of end of line whitespace mess in jsonb_indent docs.
Otherwise everything I tried so far works as expected. The code looks ok as well except maybe the replacePath could use couple of comments (for example the line which uses the h_atoi) to make it easier to follow.
About the:+ /* XXX : why do we need this assertion? The functions is declared to take text[] */
+ Assert(ARR_ELEMTYPE(path) == TEXTOID);
I wonder about this also, some functions do that, some don't, I am not really sure what is the rule there myself.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
pgsql-hackers by date: