Обсуждение: new json funcs
Here is a patch for the new json functions I mentioned a couple of
months ago. These are:
json_to_record
json_to_recordset
json_object
json_build_array
json_build_object
json_object_agg
So far there are no docs, but the way these work is illustrated in the
regression tests - I hope to have docs within a few days.
cheers
andrew
Вложения
Is it just me, or is the json_array_element(json, int) function not documented? (Not a bug in this patch, I think ...) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 01/10/2014 12:42 PM, Alvaro Herrera wrote: > Is it just me, or is the json_array_element(json, int) function not > documented? > > (Not a bug in this patch, I think ...) > As discussed at the time, we didn't document the functions underlying the json operators, just the operators themselves. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 01/10/2014 12:42 PM, Alvaro Herrera wrote:
>> Is it just me, or is the json_array_element(json, int) function not
>> documented?
> As discussed at the time, we didn't document the functions underlying
> the json operators, just the operators themselves.
I see though that json_array_element has a DESCR comment. I believe
project policy is that if a function is not meant to be invoked by name
but only through an operator, its pg_description entry should just be
"implementation of xyz operator", with the real comment attached only
to the operator. Otherwise \df users are likely to be misled into using
the function when they're not really supposed to; and at the very least
they will bitch about its lack of documentation.
See commits 94133a935414407920a47d06a6e22734c974c3b8 and
908ab80286401bb20a519fa7dc7a837631f20369.
regards, tom lane
On 01/10/2014 01:27 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 01/10/2014 12:42 PM, Alvaro Herrera wrote: >>> Is it just me, or is the json_array_element(json, int) function not >>> documented? >> As discussed at the time, we didn't document the functions underlying >> the json operators, just the operators themselves. > I see though that json_array_element has a DESCR comment. I believe > project policy is that if a function is not meant to be invoked by name > but only through an operator, its pg_description entry should just be > "implementation of xyz operator", with the real comment attached only > to the operator. Otherwise \df users are likely to be misled into using > the function when they're not really supposed to; and at the very least > they will bitch about its lack of documentation. > > See commits 94133a935414407920a47d06a6e22734c974c3b8 and > 908ab80286401bb20a519fa7dc7a837631f20369. > > OK, I can fix that I guess. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 01/10/2014 01:27 PM, Tom Lane wrote:
>> See commits 94133a935414407920a47d06a6e22734c974c3b8 and
>> 908ab80286401bb20a519fa7dc7a837631f20369.
> OK, I can fix that I guess.
Sure, just remove the DESCR comments for the functions that aren't meant
to be used directly.
I don't think this is back-patchable, but it's a minor point, so at least
for me a fix in HEAD is sufficient.
I wonder whether we should add an opr_sanity test verifying that operator
implementation functions don't have their own comments? The trouble is
that there are a few that are supposed to, but maybe that list is stable
enough that it'd be okay to memorialize in the expected output.
regards, tom lane
Andrew Dunstan wrote: > > On 01/10/2014 12:42 PM, Alvaro Herrera wrote: > >Is it just me, or is the json_array_element(json, int) function not > >documented? > > As discussed at the time, we didn't document the functions > underlying the json operators, just the operators themselves. Oh, I see. That's fine with me. From the source code it's hard to see when a SQL-callable function is only there to implement an operator, though (and it seems a bit far-fetched to suppose that the developer will think, upon seeing an undocumented function, "oh this must implement some operator, I will look it up at pg_proc.h"). I think the operator(s) should be mentioned in the comment on top of the function. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Tom Lane wrote: > I wonder whether we should add an opr_sanity test verifying that operator > implementation functions don't have their own comments? The trouble is > that there are a few that are supposed to, but maybe that list is stable > enough that it'd be okay to memorialize in the expected output. +1. It's an easy rule to overlook. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 01/10/2014 01:58 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 01/10/2014 01:27 PM, Tom Lane wrote: >>> See commits 94133a935414407920a47d06a6e22734c974c3b8 and >>> 908ab80286401bb20a519fa7dc7a837631f20369. >> OK, I can fix that I guess. > Sure, just remove the DESCR comments for the functions that aren't meant > to be used directly. > > I don't think this is back-patchable, but it's a minor point, so at least > for me a fix in HEAD is sufficient. > > I wonder whether we should add an opr_sanity test verifying that operator > implementation functions don't have their own comments? The trouble is > that there are a few that are supposed to, but maybe that list is stable > enough that it'd be okay to memorialize in the expected output. > > Well, that would be ok as long as there was a comment in the file so that developers don't just think it's OK to extend the list (it's a bit like part of the reason we don't allow shift/reduce conflicts - if we allowed them people would just keep adding more, and they wouldn't stick out like a sore thumb.) The comment in the current test says: -- Check that operators' underlying functions have suitable comments, -- namely 'implementation of XXX operator'. Insome cases involving legacy -- names for operators, there are multiple operators referencing the same -- pg_procentry, so ignore operators whose comments say they are deprecated. -- We also have a few functions that are bothoperator support and meant to -- be called directly; those should have comments matching their operator. The history here is that originally I was intending to have these functions documented, and so the descriptions were made to match the operator descriptions, so that we didn't get a failure on this test. Later we decided not to document them as part of last release's bike-shedding, but the function descriptions didn't get changed / removed. cheers andrew
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Oh, I see. That's fine with me. From the source code it's hard to see
> when a SQL-callable function is only there to implement an operator,
> though (and it seems a bit far-fetched to suppose that the developer
> will think, upon seeing an undocumented function, "oh this must
> implement some operator, I will look it up at pg_proc.h").
> I think the operator(s) should be mentioned in the comment on top of the
> function.
Oh, you're complaining about the lack of any header comment for the
function in the source code. That's a different matter from the
user-visible docs, but I agree that it's poor practice to not have
anything.
regards, tom lane
Andrew Dunstan <andrew@dunslane.net> writes:
> The history here is that originally I was intending to have these
> functions documented, and so the descriptions were made to match the
> operator descriptions, so that we didn't get a failure on this test.
> Later we decided not to document them as part of last release's
> bike-shedding, but the function descriptions didn't get changed / removed.
Ah. I suppose there's no way to cross-check the state of the function's
pg_description comment against whether it has SGML documentation :-(
regards, tom lane
On Fri, Jan 10, 2014 at 02:39:12PM -0500, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > The history here is that originally I was intending to have these > > functions documented, and so the descriptions were made to match the > > operator descriptions, so that we didn't get a failure on this test. > > Later we decided not to document them as part of last release's > > bike-shedding, but the function descriptions didn't get changed / removed. > > Ah. I suppose there's no way to cross-check the state of the function's > pg_description comment against whether it has SGML documentation :-( FDWs to the rescue! Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> I wonder whether we should add an opr_sanity test verifying that operator
>> implementation functions don't have their own comments? The trouble is
>> that there are a few that are supposed to, but maybe that list is stable
>> enough that it'd be okay to memorialize in the expected output.
> +1. It's an easy rule to overlook.
Here's a proposed addition to opr_sanity.sql for that:
-- Show all the operator-implementation functions that have their own
-- comments. This should happen only for cases where the function and
-- operator syntaxes are equally preferred (and are both documented).
-- Because it's a pretty short list, it's okay to have all the occurrences
-- appearing in the expected output.
WITH funcdescs AS ( SELECT p.oid as p_oid, proname, o.oid as o_oid, obj_description(p.oid, 'pg_proc') as prodesc,
'implementationof ' || oprname || ' operator' as expecteddesc, obj_description(o.oid, 'pg_operator') as oprdesc FROM
pg_procp JOIN pg_operator o ON oprcode = p.oid WHERE o.oid <= 9999
)
SELECT p_oid, proname, prodesc FROM funcdescs WHERE prodesc IS DISTINCT FROM expecteddesc AND oprdesc NOT LIKE
'deprecated%'
ORDER BY 1;
In HEAD, this query produces
p_oid | proname | prodesc
-------+---------------------------+------------------------------------------------ 378 | array_append |
appendelement onto end of array 379 | array_prepend | prepend element onto front of array 1035 | aclinsert
| add/update ACL item 1036 | aclremove | remove ACL item 1037 | aclcontains
| contains 3947 | json_object_field | get json object field 3948 | json_object_field_text | get json object
fieldas text 3949 | json_array_element | get json array element 3950 | json_array_element_text | get json
arrayelement as text 3952 | json_extract_path_op | get value from json with path elements 3954 |
json_extract_path_text_op| get value from json as text with path elements
(11 rows)
The first five of these, I believe, are the cases I left alone back in
commit 94133a935414407920a47d06a6e22734c974c3b8. I'm thinking the
other six are the ones Andrew needs to remove the DESCR entries for.
regards, tom lane
On 01/10/2014 07:16 PM, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> Tom Lane wrote: >>> I wonder whether we should add an opr_sanity test verifying that operator >>> implementation functions don't have their own comments? The trouble is >>> that there are a few that are supposed to, but maybe that list is stable >>> enough that it'd be okay to memorialize in the expected output. >> +1. It's an easy rule to overlook. > Here's a proposed addition to opr_sanity.sql for that: > > -- Show all the operator-implementation functions that have their own > -- comments. This should happen only for cases where the function and > -- operator syntaxes are equally preferred (and are both documented). > -- Because it's a pretty short list, it's okay to have all the occurrences > -- appearing in the expected output. > WITH funcdescs AS ( > SELECT p.oid as p_oid, proname, o.oid as o_oid, > obj_description(p.oid, 'pg_proc') as prodesc, > 'implementation of ' || oprname || ' operator' as expecteddesc, > obj_description(o.oid, 'pg_operator') as oprdesc > FROM pg_proc p JOIN pg_operator o ON oprcode = p.oid > WHERE o.oid <= 9999 > ) > SELECT p_oid, proname, prodesc FROM funcdescs > WHERE prodesc IS DISTINCT FROM expecteddesc > AND oprdesc NOT LIKE 'deprecated%' > ORDER BY 1; > > In HEAD, this query produces > > p_oid | proname | prodesc > -------+---------------------------+------------------------------------------------ > 378 | array_append | append element onto end of array > 379 | array_prepend | prepend element onto front of array > 1035 | aclinsert | add/update ACL item > 1036 | aclremove | remove ACL item > 1037 | aclcontains | contains > 3947 | json_object_field | get json object field > 3948 | json_object_field_text | get json object field as text > 3949 | json_array_element | get json array element > 3950 | json_array_element_text | get json array element as text > 3952 | json_extract_path_op | get value from json with path elements > 3954 | json_extract_path_text_op | get value from json as text with path elements > (11 rows) > > The first five of these, I believe, are the cases I left alone back in > commit 94133a935414407920a47d06a6e22734c974c3b8. I'm thinking the > other six are the ones Andrew needs to remove the DESCR entries for. > > Yeah, you just knocked out the last condition in the where clause, right? Confirmed that when I do that and remove those DESCR entries we're left with those 5 rows. Is it better to knock out the DESCR entries totally or make them read "implementation of foo operator"? cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Is it better to knock out the DESCR entries totally or make them read
> "implementation of foo operator"?
Just delete them --- initdb is responsible for inserting that boilerplate
where appropriate.
regards, tom lane
On 1/3/14, 9:00 PM, Andrew Dunstan wrote: > > Here is a patch for the new json functions I mentioned a couple of > months ago. These are: > > json_to_record > json_to_recordset > json_object > json_build_array > json_build_object > json_object_agg > > So far there are no docs, but the way these work is illustrated in the > regression tests - I hope to have docs within a few days. Compiler warnings: json.c: In function ‘json_object_two_arg’: json.c:2210:5: warning: unused variable ‘count’ [-Wunused-variable] jsonfuncs.c: In function ‘json_to_record’: jsonfuncs.c:1955:16: warning: unused variable ‘tuple’ [-Wunused-variable] jsonfuncs.c:1953:18: warning: variable ‘rec’ set but not used [-Wunused-but-set-variable] Also, please run your patch through git diff --check. I have noticed that several of your patches have hilarious whitespace, maybe something with your editor.
On 01/16/2014 01:57 PM, Peter Eisentraut wrote: > On 1/3/14, 9:00 PM, Andrew Dunstan wrote: >> Here is a patch for the new json functions I mentioned a couple of >> months ago. These are: >> >> json_to_record >> json_to_recordset >> json_object >> json_build_array >> json_build_object >> json_object_agg >> >> So far there are no docs, but the way these work is illustrated in the >> regression tests - I hope to have docs within a few days. > Compiler warnings: > > json.c: In function ‘json_object_two_arg’: > json.c:2210:5: warning: unused variable ‘count’ [-Wunused-variable] > > jsonfuncs.c: In function ‘json_to_record’: > jsonfuncs.c:1955:16: warning: unused variable ‘tuple’ [-Wunused-variable] > jsonfuncs.c:1953:18: warning: variable ‘rec’ set but not used [-Wunused-but-set-variable] > > Also, please run your patch through git diff --check. I have noticed > that several of your patches have hilarious whitespace, maybe > something with your editor. > I'm happy to keep you amused. Some of this was probably due to cutting and pasting. all these issues are fixed in the attached patch. cheers andrew
Вложения
On 01/16/2014 07:39 PM, Andrew Dunstan wrote: > > On 01/16/2014 01:57 PM, Peter Eisentraut wrote: >> On 1/3/14, 9:00 PM, Andrew Dunstan wrote: >>> Here is a patch for the new json functions I mentioned a couple of >>> months ago. These are: >>> >>> json_to_record >>> json_to_recordset >>> json_object >>> json_build_array >>> json_build_object >>> json_object_agg >>> >>> So far there are no docs, but the way these work is illustrated in the >>> regression tests - I hope to have docs within a few days. >> Compiler warnings: >> >> json.c: In function ‘json_object_two_arg’: >> json.c:2210:5: warning: unused variable ‘count’ [-Wunused-variable] >> >> jsonfuncs.c: In function ‘json_to_record’: >> jsonfuncs.c:1955:16: warning: unused variable ‘tuple’ >> [-Wunused-variable] >> jsonfuncs.c:1953:18: warning: variable ‘rec’ set but not used >> [-Wunused-but-set-variable] >> >> Also, please run your patch through git diff --check. I have noticed >> that several of your patches have hilarious whitespace, maybe >> something with your editor. >> > > > I'm happy to keep you amused. Some of this was probably due to cutting > and pasting. > > all these issues are fixed in the attached patch. In case anyone feels like really nitpicking, this version fixes some pgindent weirdness due to an outdated typedef list in the previous version. cheers andrew
Вложения
On 2014-01-17 03:08, Andrew Dunstan wrote: > In case anyone feels like really nitpicking, this version fixes some > pgindent weirdness due to an outdated typedef list in the previous version. Is it possible for you to generate a diff that doesn't have all these unrelated changes in it (from a pgindent run, I presume)? I just read through three pagefuls and I didn't see any relevant changes, but I'm not sure I want to keep doing that for the rest of the patch. Regards, Marko Tiikkaja
On 01/18/2014 12:34 PM, Marko Tiikkaja wrote: > On 2014-01-17 03:08, Andrew Dunstan wrote: >> In case anyone feels like really nitpicking, this version fixes some >> pgindent weirdness due to an outdated typedef list in the previous >> version. > > Is it possible for you to generate a diff that doesn't have all these > unrelated changes in it (from a pgindent run, I presume)? I just read > through three pagefuls and I didn't see any relevant changes, but I'm > not sure I want to keep doing that for the rest of the patch. > > > This seems to be quite overstated. The chunks in the version 3 patch that only contain pgindent effects are those at lines 751,771,866 and 1808 of json.c, by my reckoning. All the other changes are more than formatting. And undoing the pgindent changes and then individually applying all but those mentioned above would take me a lot of time. cheers andrew
On 1/18/14, 9:38 PM, Andrew Dunstan wrote: > On 01/18/2014 12:34 PM, Marko Tiikkaja wrote: >> Is it possible for you to generate a diff that doesn't have all these >> unrelated changes in it (from a pgindent run, I presume)? I just read >> through three pagefuls and I didn't see any relevant changes, but I'm >> not sure I want to keep doing that for the rest of the patch. >> > > This seems to be quite overstated. The chunks in the version 3 patch > that only contain pgindent effects are those at lines 751,771,866 and > 1808 of json.c, by my reckoning. All the other changes are more than > formatting. Oh I see, there's a version 3 which improves things by a lot. I just took the latest patch from the CF app and that was the v2 patch. Now looking at it again, I see that it actually added new lines around json.c:68, which I believe proves my point that reviewing such a patch is hard. > And undoing the pgindent changes and then individually applying all but > those mentioned above would take me a lot of time. v3 looks "ok". I would have preferred a patch with no unrelated changes, but I can live with what we have there. Something like the first three pagefuls of v2, however, would take *me* a lot of time, which I believe is not acceptable. I don't care why a patch has lots of unrelated stuff in it, I'm not going to waste my time trying to figure out which parts are relevant and which aren't. That's a lot of time wasted just to end up with a review possibly full of missed problems and misunderstood code. But I'll continue with my review now that this has been sorted out. Regards, Marko Tiikkaja
Hi Andrew, On 1/18/14, 10:05 PM, I wrote: > But I'll continue with my review now that this has been sorted out. Sorry about the delay. I think the API for the new functions looks good. They are all welcome additions to the JSON family. The implementation side looks reasonable to me. I'm not sure there's need to duplicate so much code, though. E.g. json_to_recordset is almost identical to json_populate_recordset, and json_to_record has a bit of the same disease. Finally, (as I'm sure you know already), docs are still missing. Marking the patch Waiting on Author for the time being. Regards, Marko Tiikkaja
On 01/21/2014 06:21 PM, Marko Tiikkaja wrote: > Hi Andrew, > > On 1/18/14, 10:05 PM, I wrote: >> But I'll continue with my review now that this has been sorted out. > > Sorry about the delay. > > I think the API for the new functions looks good. They are all > welcome additions to the JSON family. > > The implementation side looks reasonable to me. I'm not sure there's > need to duplicate so much code, though. E.g. json_to_recordset is > almost identical to json_populate_recordset, and json_to_record has a > bit of the same disease. I can probably factor some of that out. Of course, when it was an extension there wasn't the possibility. > > Finally, (as I'm sure you know already), docs are still missing. > Marking the patch Waiting on Author for the time being. > > Yes, I have a draft, just waiting for time to go through it. Thanks for the review. cheers andrew
On 01/21/2014 06:21 PM, Marko Tiikkaja wrote: > Hi Andrew, > > On 1/18/14, 10:05 PM, I wrote: >> But I'll continue with my review now that this has been sorted out. > > Sorry about the delay. > > I think the API for the new functions looks good. They are all > welcome additions to the JSON family. > > The implementation side looks reasonable to me. I'm not sure there's > need to duplicate so much code, though. E.g. json_to_recordset is > almost identical to json_populate_recordset, and json_to_record has a > bit of the same disease. > > Finally, (as I'm sure you know already), docs are still missing. > Marking the patch Waiting on Author for the time being. > > > New patch attached. Main change is I changed json_populate_record/json_to_record to call a common worker function, and likewise with json_populate_recordset/json_to_recordset. We're still finalizing the docs - should be ready in the next day or so. cheers andrew
Вложения
On 01/22/2014 12:49 PM, Andrew Dunstan wrote:
>
> On 01/21/2014 06:21 PM, Marko Tiikkaja wrote:
>> Hi Andrew,
>>
>> On 1/18/14, 10:05 PM, I wrote:
>>> But I'll continue with my review now that this has been sorted out.
>>
>> Sorry about the delay.
>>
>> I think the API for the new functions looks good. They are all
>> welcome additions to the JSON family.
>>
>> The implementation side looks reasonable to me. I'm not sure there's
>> need to duplicate so much code, though. E.g. json_to_recordset is
>> almost identical to json_populate_recordset, and json_to_record has a
>> bit of the same disease.
>>
>> Finally, (as I'm sure you know already), docs are still missing.
>> Marking the patch Waiting on Author for the time being.
>>
>>
>>
>
>
> New patch attached. Main change is I changed
> json_populate_record/json_to_record to call a common worker function,
> and likewise with json_populate_recordset/json_to_recordset.
>
> We're still finalizing the docs - should be ready in the next day or so.
OK, here's the patch, this time with docs, thanks to Merlin Moncure and
Josh Berkus for help with that.
I want to do some more wordsmithing around json_to_record{set} and
json_populate_record{set}, but I think this is close to being
committable as is.
cheers
andrew
Вложения
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:13px">For consistency with the existing json
functions(json_each, json_each_text, etc.) it might be better to add separate json_to_record_text and
json_to_recordset_textfunctions in place of the nested_as_text parameter to json_to_record
and json_to_recordset.</span><br/></div><div class="gmail_extra"><br /><br /><div class="gmail_quote">On 24 January
201410:26, Andrew Dunstan <span dir="ltr"><<a href="mailto:andrew@dunslane.net"
target="_blank">andrew@dunslane.net</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px#ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br /> On 01/22/2014 12:49 PM,
AndrewDunstan wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc
solid;padding-left:1ex"><br/> On 01/21/2014 06:21 PM, Marko Tiikkaja wrote:<br /><blockquote class="gmail_quote"
style="margin:00 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Hi Andrew,<br /><br /> On 1/18/14, 10:05 PM, I
wrote:<br/><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> But
I'llcontinue with my review now that this has been sorted out.<br /></blockquote><br /> Sorry about the delay.<br /><br
/>I think the API for the new functions looks good. They are all welcome additions to the JSON family.<br /><br /> The
implementationside looks reasonable to me. I'm not sure there's need to duplicate so much code, though. E.g.
json_to_recordsetis almost identical to json_populate_recordset, and json_to_record has a bit of the same disease.<br
/><br/> Finally, (as I'm sure you know already), docs are still missing. Marking the patch Waiting on Author for the
timebeing.<br /><br /><br /><br /></blockquote><br /><br /> New patch attached. Main change is I changed
json_populate_record/json_to_<u></u>recordto call a common worker function, and likewise with
json_populate_recordset/json_<u></u>to_recordset.<br/><br /> We're still finalizing the docs - should be ready in the
nextday or so.<br /></blockquote><br /><br /></div></div> OK, here's the patch, this time with docs, thanks to Merlin
Moncureand Josh Berkus for help with that.<br /><br /> I want to do some more wordsmithing around json_to_record{set}
andjson_populate_record{set}, but I think this is close to being committable as is.<br /><br /> cheers<span
class="HOEnZb"><fontcolor="#888888"><br /><br /> andrew<br /><br /><br /></font></span><br /><br /> --<br /> Sent via
pgsql-hackersmailing list (<a href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br /> To
makechanges to your subscription:<br /><a href="http://www.postgresql.org/mailpref/pgsql-hackers"
target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/><br /></blockquote></div><br /></div>
On 01/24/2014 03:40 PM, Laurence Rowe wrote: > For consistency with the existing json functions (json_each, > json_each_text, etc.) it might be better to add separate > json_to_record_text and json_to_recordset_text functions in place of > the nested_as_text parameter to json_to_record and json_to_recordset. > > It wouldn't be consistent with json_populate_record() and json_populate_recordset(), the two closest relatives, however. And yes, I appreciate that we have not been 100% consistent. Community design can be a bit messy that way. cheers andrew
On 01/24/2014 12:59 PM, Andrew Dunstan wrote: > > On 01/24/2014 03:40 PM, Laurence Rowe wrote: >> For consistency with the existing json functions (json_each, >> json_each_text, etc.) it might be better to add separate >> json_to_record_text and json_to_recordset_text functions in place of >> the nested_as_text parameter to json_to_record and json_to_recordset. >> >> > > It wouldn't be consistent with json_populate_record() and > json_populate_recordset(), the two closest relatives, however. > > And yes, I appreciate that we have not been 100% consistent. Community > design can be a bit messy that way. FWIW, I prefer the parameter to having differently named functions. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Fri, Jan 24, 2014 at 3:26 PM, Josh Berkus <josh@agliodbs.com> wrote: > On 01/24/2014 12:59 PM, Andrew Dunstan wrote: >> >> On 01/24/2014 03:40 PM, Laurence Rowe wrote: >>> For consistency with the existing json functions (json_each, >>> json_each_text, etc.) it might be better to add separate >>> json_to_record_text and json_to_recordset_text functions in place of >>> the nested_as_text parameter to json_to_record and json_to_recordset. >>> >>> >> >> It wouldn't be consistent with json_populate_record() and >> json_populate_recordset(), the two closest relatives, however. >> >> And yes, I appreciate that we have not been 100% consistent. Community >> design can be a bit messy that way. > > FWIW, I prefer the parameter to having differently named functions. +1. merlin
On 01/27/2014 12:43 PM, Merlin Moncure wrote: > On Fri, Jan 24, 2014 at 3:26 PM, Josh Berkus <josh@agliodbs.com> wrote: >> On 01/24/2014 12:59 PM, Andrew Dunstan wrote: >>> On 01/24/2014 03:40 PM, Laurence Rowe wrote: >>>> For consistency with the existing json functions (json_each, >>>> json_each_text, etc.) it might be better to add separate >>>> json_to_record_text and json_to_recordset_text functions in place of >>>> the nested_as_text parameter to json_to_record and json_to_recordset. >>>> >>>> >>> It wouldn't be consistent with json_populate_record() and >>> json_populate_recordset(), the two closest relatives, however. >>> >>> And yes, I appreciate that we have not been 100% consistent. Community >>> design can be a bit messy that way. >> FWIW, I prefer the parameter to having differently named functions. > +1. > Note that we can only do this when the result type stays the same. It does not for json_each/json_each_text or json_extract_path/json_extract_path_text, which is why we have different functions for those cases. cheers andrew
Andrew Dunstan escribió: > Note that we can only do this when the result type stays the same. > It does not for json_each/json_each_text or > json_extract_path/json_extract_path_text, which is why we have > different functions for those cases. In C code, if I extract a value using json_object_field or json_array_element, is there a way to turn it into the dequoted version, that is, the value that I would have gotten had I called json_object_field_text or json_array_element_text instead? I wrote a quick and dirty hack in the event triggers patch that just removes the outermost "" and turns any \" into ", but that's probably incomplete. Does jsonfuncs.c offer any way to do this? That might be useful for the crowd that cares about the detail being discussed in this subthread. Thanks, -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 01/27/2014 03:53 PM, Alvaro Herrera wrote: > Andrew Dunstan escribió: > >> Note that we can only do this when the result type stays the same. >> It does not for json_each/json_each_text or >> json_extract_path/json_extract_path_text, which is why we have >> different functions for those cases. > In C code, if I extract a value using json_object_field or > json_array_element, is there a way to turn it into the dequoted version, > that is, the value that I would have gotten had I called > json_object_field_text or json_array_element_text instead? > > I wrote a quick and dirty hack in the event triggers patch that just > removes the outermost "" and turns any \" into ", but that's probably > incomplete. Does jsonfuncs.c offer any way to do this? That might be > useful for the crowd that cares about the detail being discussed in this > subthread. > I'm not sure I understand the need. This is the difference between the _text variants and their parents. Why would you call json_object_field when you want the dequoted text? cheers andrew
Andrew Dunstan escribió: > I'm not sure I understand the need. This is the difference between > the _text variants and their parents. Why would you call > json_object_field when you want the dequoted text? Because I first need to know its type. Sometimes it's an array, or an object, or a boolean, and for those I won't call the _text version afterwards but just use the original. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 01/27/2014 01:06 PM, Alvaro Herrera wrote: > Andrew Dunstan escribió: > >> I'm not sure I understand the need. This is the difference between >> the _text variants and their parents. Why would you call >> json_object_field when you want the dequoted text? > > Because I first need to know its type. Sometimes it's an array, or an > object, or a boolean, and for those I won't call the _text version > afterwards but just use the original. It would make more sense to extract them as JSON, check the type, and convert. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Josh Berkus wrote:
> On 01/27/2014 01:06 PM, Alvaro Herrera wrote:
> > Andrew Dunstan escribió:
> >
> >> I'm not sure I understand the need. This is the difference between
> >> the _text variants and their parents. Why would you call
> >> json_object_field when you want the dequoted text?
> >
> > Because I first need to know its type. Sometimes it's an array, or an
> > object, or a boolean, and for those I won't call the _text version
> > afterwards but just use the original.
>
> It would make more sense to extract them as JSON, check the type, and
> convert.
That's what I'm already doing. My question is how do I convert it?
I have this, but would like to get rid of it:
/** dequote_jsonval* Take a string value extracted from a JSON object, and return a copy of it* with the
quotingremoved.** Another alternative to this would be to run the extraction routine again,* using the "_text" variant
whichreturns the value without quotes; but this* complicates the logic a lot because not all values are extracted in*
thesame way (some are extracted using json_object_field, others* using json_array_element). Dequoting the object
alreadyat hand is a* lot easier.*/
static char *
dequote_jsonval(char *jsonval)
{char *result;int inputlen = strlen(jsonval);int i;int j = 0;
result = palloc(strlen(jsonval) + 1);
/* skip the start and end quotes right away */for (i = 1; i < inputlen - 1; i++){ /* * XXX this skips the \ in a
\"sequence but leaves other escaped * sequences in place. Are there other cases we need to handle * specially?
*/ if (jsonval[i] == '\\' && jsonval[i + 1] == '"') { i++; continue; }
result[j++] = jsonval[i];}result[j] = '\0';
return result;
}
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 01/28/2014 01:22 PM, Alvaro Herrera wrote:
> Josh Berkus wrote:
>> On 01/27/2014 01:06 PM, Alvaro Herrera wrote:
>>> Andrew Dunstan escribió:
>>>
>>>> I'm not sure I understand the need. This is the difference between
>>>> the _text variants and their parents. Why would you call
>>>> json_object_field when you want the dequoted text?
>>> Because I first need to know its type. Sometimes it's an array, or an
>>> object, or a boolean, and for those I won't call the _text version
>>> afterwards but just use the original.
>> It would make more sense to extract them as JSON, check the type, and
>> convert.
> That's what I'm already doing. My question is how do I convert it?
> I have this, but would like to get rid of it:
>
> /*
> * dequote_jsonval
> * Take a string value extracted from a JSON object, and return a copy of it
> * with the quoting removed.
> *
> * Another alternative to this would be to run the extraction routine again,
> * using the "_text" variant which returns the value without quotes; but this
> * complicates the logic a lot because not all values are extracted in
> * the same way (some are extracted using json_object_field, others
> * using json_array_element). Dequoting the object already at hand is a
> * lot easier.
> */
> static char *
> dequote_jsonval(char *jsonval)
> {
> char *result;
> int inputlen = strlen(jsonval);
> int i;
> int j = 0;
>
> result = palloc(strlen(jsonval) + 1);
>
> /* skip the start and end quotes right away */
> for (i = 1; i < inputlen - 1; i++)
> {
> /*
> * XXX this skips the \ in a \" sequence but leaves other escaped
> * sequences in place. Are there other cases we need to handle
> * specially?
> */
> if (jsonval[i] == '\\' &&
> jsonval[i + 1] == '"')
> {
> i++;
> continue;
> }
>
> result[j++] = jsonval[i];
> }
> result[j] = '\0';
>
> return result;
> }
>
Well, TIMTOWTDI. Here's roughly what I would do, in json.c, making the
json lexer do the work for us:
extern char * dequote_scalar_json_string(char *jsonval) { text *json = cstring_to_text(jsonval);
JsonLexContext*lex = makeJsonLexContext(json, true); JsonTokenType tok; char *ret;
json_lex(lex); tok = lex_peek(lex); if (tok == JSON_TOKEN_STRING)
ret=pstrdup(lex->strval->data); else ret = jsonval; pfree(lex->strval->data);
pfree(lex->strval); pfree(lex); pfree(json);
return ret; }
I'm not sure if we should have a gadget like this at the SQL level also.
cheers
andrew
Hi Andrew,
On 1/24/14, 7:26 PM, Andrew Dunstan wrote:
> OK, here's the patch, this time with docs, thanks to Merlin Moncure and
> Josh Berkus for help with that.
Thanks, this one is looking pretty good. A couple of small issues:
- The oid 3195 of json_object_agg_transfn has been taken by a recent
commit, so that had to be changed. The patch compiled and passed tests
after that.
- Typo in the description of json_build_array: "agument list"
- I find (perhaps due to not being a native speaker) the description
of json_object a bit painful to read. I would've expected something like:
- Builds a JSON object out of a text array. The array must have
exactly one dimension
+ Builds a JSON object out of a text array. The array must have
either exactly one dimension with an even number of members, in which case they are taken
as alternating name/value
- pairs, or two dimensions with such that each inner array has
exactly two elements, which
+ pairs, or two dimensions such that each inner array has
exactly two elements, which are taken as a name/value pair.
but I'm not sure about that either.
- There are a few cases of curly braces around a single-statement
else, which I believe is against the project's code style guidelines.
Otherwise this patch looks good to my eyes.
Regards,
Marko Tiikkaja
On 01/28/2014 05:07 PM, Marko Tiikkaja wrote: > Hi Andrew, > > On 1/24/14, 7:26 PM, Andrew Dunstan wrote: >> OK, here's the patch, this time with docs, thanks to Merlin Moncure and >> Josh Berkus for help with that. > > Thanks, this one is looking pretty good. A couple of small issues: > > - The oid 3195 of json_object_agg_transfn has been taken by a recent > commit, so that had to be changed. The patch compiled and passed > tests after that. Yeah. These days you can't even build if there's a duplicate oid, so fixing that and a catalog version bump would be part of committing. > > - Typo in the description of json_build_array: "agument list" will fix. > > - I find (perhaps due to not being a native speaker) the description > of json_object a bit painful to read. I would've expected something > like: > > - Builds a JSON object out of a text array. The array must > have exactly one dimension > + Builds a JSON object out of a text array. The array must > have either exactly one dimension > with an even number of members, in which case they are taken > as alternating name/value > - pairs, or two dimensions with such that each inner array has > exactly two elements, which > + pairs, or two dimensions such that each inner array has > exactly two elements, which > are taken as a name/value pair. > > but I'm not sure about that either. Yes, yours looks better. > > - There are a few cases of curly braces around a single-statement > else, which I believe is against the project's code style guidelines. IIRC we actually stopped pgindent removing that quite a few years ago, and the formatting guidelines at <http://www.postgresql.org/docs/devel/static/source-format.html> don't say anything about it. Personally, I prefer consistency - I think either both branches of an if/else should use curly braces or neither should. I find it quite ugly and jarring when one does and the other doesn't. Thanks for the review. cheers andrew