On 02/24/2013 02:15 AM, Craig Ringer wrote:
On 02/24/2013 02:09 PM, Steve Singer wrote:
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.
I'm not thrilled about that, because we're likely to want to add more
JSON-specific casts to built-in or extension types in the future. If
doing so changes behaviour, causing something that used to work to
continue to work but produce a different result, that'll result in
considerable arguments about backward compatibility.
I'd be happier to require explicit casts to text or require the user to
explicitly CREATE CAST where no JSON-aware cast is already defined.
Adding a cast to json for a builtin type will have no effect unless you
also change this code. We can relax that but my view was that we should
know how to generate JSON from builtin types and just do it. But if we
wanted to allow someone to create a cast from, say, xml to json, and
have it take effect in json_agg then it might make sense to honor casts
for all types.
Your requirement of an explicit cast to text or json would result in a
class of type that could not be represented as json at all. I'm very
strongly opposed to that. If you proposed instead to prefer a cast to
text and only fall back on the type's default output format if one
doesn't exist I could live with that, although I strongly suspect it
will be mostly pointless.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
