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
diff --git a/src/include/catalog/pg_aggregate.h b/src/include/catalog/pg_aggregate.h
index 3c5f59b..6fb10a9 100644
*** a/src/include/catalog/pg_aggregate.h
--- b/src/include/catalog/pg_aggregate.h
*************** DATA(insert ( 3538 string_agg_transfn st
*** 233,239 ****
DATA(insert ( 3545 bytea_string_agg_transfn bytea_string_agg_finalfn 0 2281 _null_ ));
/* json */
! DATA(insert ( 3172 json_agg_transfn json_agg_finalfn 0 2281 _null_ ));
/*
* prototypes for functions in pg_aggregate.c
--- 233,239 ----
DATA(insert ( 3545 bytea_string_agg_transfn bytea_string_agg_finalfn 0 2281 _null_ ));
/* json */
! DATA(insert ( 3175 json_agg_transfn json_agg_finalfn 0 2281 _null_ ));
/*
* prototypes for functions in pg_aggregate.c
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
