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
>
>