Re: json generation enhancements

On 13-02-25 05:37 PM, Andrew Dunstan wrote:
>
> On 02/24/2013 01:09 AM, Steve Singer wrote:
>> On 13-01-11 11:03 AM, Andrew Dunstan wrote:
>>>
>>> On 01/11/2013 11:00 AM, Andrew Dunstan wrote:
>>>>
>>>> I have not had anyone follow up on this, so I have added docs and 
>>>> will add this to the commitfest.
>>>>
>>>> Recap:
>>>>
>>>> This adds the following:
>>>>
>>>>     json_agg(anyrecord) -> json
>>>>     to_json(any) -> json
>>>>     hstore_to_json(hstore) -> json (also used as a cast)
>>>>     hstore_to_json_loose(hstore) -> json
>>>>
>>>> Also, in json generation, if any non-builtin type has a cast to 
>>>> json, that function is used instead of the type's output function.
>>>>
>>>>
>>>
>>> This time with a patch.
>>>
>>
>> Here is a review of this patch.,
>>
>> Overview
>> ---------------------
>> This patch adds a set of functions to convert types to json. 
>> Specifically
>>
>> * An aggregate that take the elements and builds up a json array.
>> * Conversions from an hstore to json
>>
>> For non-builtin types the text conversion is used unless a cast has 
>> specifically been defined from the type to json.
>>
>> There was some discussion last year on this patch that raised three 
>> issues
>>
>> a) Robert was concerned that if someone added a cast from a non-core 
>> type like citext to json that it would change the behaviour of this 
>> function. No one else offered an opinion on this at the time.  I 
>> don't see this as a problem, if I add a cast between citext (or any 
>> other non-core datatype) to json I would expect it to effect how that 
>> datatype is generated as a json object in any functions that generate 
>> json.   I don't see why this would be surprising behaviour.  If I add 
>> a cast between my datatype and json to generate a json representation 
>> that differs from the textout representation then I would expect that 
>> representation to be used in the json_agg function as well.
>>
>> b) There was some talk in the json bikeshedding thread that 
>> json_agg() should be renamed to collect_json() but more people 
>> preferred json_agg().
>>
>> c) Should an aggregate of an empty result produce NULL or an empty 
>> json element. Most people who commented on the thread seemed to feel 
>> that NULL is preferred because it is  consistent with other aggregates
>>
>> I feel that the functionality of this patch is fine.
>>
>> Testing
>> -------------------
>>
>> When I try running the regression tests with this patch I get:
>> creating template1 database in 
>> /usr/local/src/postgresql/src/test/regress/./tmp_check/data/base/1 
>> ... FATAL:  could not create unique index "pg_proc_oid_index"
>> DETAIL:  Key (oid)=(3171) is duplicated.
>> child process exited with exit code 1
>>
>> oid 3170, 3171 and 3172 appears to be used by lo_tell64 and lo_lseek64
>>
>> If I fix those up the regression tests pass.
>>
>> I tried using the new functions in a few different ways and 
>> everything worked as expected.
>>
>> Code Review
>> -----------
>> in contrib/hstore/hstore_io.c
>> +                                       /* not an int - try a double */
>> +                                       double doubleres = 
>> strtod(src->data,&endptr);
>> +                                       if (*endptr == '\0')
>> +                                               is_number = true;
>> +                                       else if (false)
>> +                                       {
>> +                                               /* shut the compiler 
>> up about unused variables */
>> +                                               longres = (long) 
>> doubleres;
>> +                                               longres = longres / 2;
>>
>>
>> I dislike that we have to do this to avoid compiler warnings.  I am 
>> also worried the some other compiler might decide that the else if 
>> (false) is worthy of a warning.  Does anyone have cleaner way of 
>> getting rid of the warning we get if we don't store the strtol/strtod 
>> result?
>>
>> Should we do something like:
>>
>> (void) ( strtol(src->data,&endptr,10)+1);
>>
>> Other than that I don't see any issues.
>>
>>
>
> Thanks for the review. Revised patch attached with fresh OIDs and 
> numeric detection code cleaned up along the lines you suggest.
>

The opr_test unit test still fails.
I think you forgot to include your update to pg_aggregate.h.  See the 
attached diff.

Other than that it looks fine, Craig is satisfied with the casting 
behaviour and no one else has objected to it.

I think your good to commit this

Steve

> cheers
>
> andrew
>
>