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 <p...@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
>
