On 01/28/2014 09:38 AM, Merlin Moncure wrote:
> On Mon, Jan 27, 2014 at 9:43 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>> On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>>>
>>> Here is the latest set of patches for nested hstore and jsonb.
>>>
>>> Because it's so large I've broken this into two patches and compressed
>>> them. The jsonb patch should work standalone. The nested hstore patch
>>> depends on it.
>>>
>>> All the jsonb functions now use the jsonb API - there is no more turning
>>> jsonb into text and reparsing it.
>>>
>>> At this stage I'm going to be starting cleanup on the jsonb code
>>> (indentation, error messages, comments etc.) as well get getting up some
>>> jsonb docs.
>>>
>>>
>>>
>>
>> Here is an update of the jsonb part of this. Charges:
>>
>> * there is now documentation for jsonb
>> * most uses of elog() in json_funcs.c are replaced by ereport().
>> * indentation fixes and other tidying.
>>
>> No changes in functionality.
> Don't have time to fire it up this morning, but a quick scan of the
> patch turned up a few minor things:
>
> * see a comment typo, line 290 'jsonn':
> * line 332: 'bogus input' -- is this up to error reporting standards?
> How about "value 'x' must be one of array, object, numeric, string,
> bool"?
> * line 357: "jsonb's key could be only a string" prefer non
> possessive: jsonb keys must be a string
> * line 374, 389: ditto 332
> * line 513: is panic appropriate here?
> * line 599: ditto
> * line 730: odd phrasing in comment, also commenting on this function
> is a little light
> * line 807: slightly prefer 'with respect to'
> * line 888: another PANIC: these maybe correct, seems odd to halt
> server on corrupted datum though*
> * line 1150: hm, is the jsonb internal hash structure documented?
> Aside: why didn't we use standard hash table (performance maybe)?
> * line 1805-6: poor phrasing. How about: "it will order and make
> unique the hash keys. Otherwise we believe that pushed keys are
> ordered and unique. (Don't like verbed 'unqiue').
> * line 1860: "no break here: "
>
Looks like this review is against jsonb-5, not jsonb-6.
cheers
andrew