Re: json generation enhancements - Mailing list pgsql-hackers
From | Steve Singer |
---|---|
Subject | Re: json generation enhancements |
Date | |
Msg-id | BLU0-SMTP1974E8ED2BA323C369001EDCFF0@phx.gbl Whole thread Raw |
In response to | Re: json generation enhancements (Andrew Dunstan <andrew@dunslane.net>) |
List | pgsql-hackers |
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 > >
Attachment
pgsql-hackers by date: