Обсуждение: proposal: ignore null fields in not relation type composite type based constructors

<div dir="ltr">Hi Pavel,<br /><br />You have said that XMLFOREST has something which ignores nulls, what's that?<br
/>Willyou please provide an example ?<br /><br />I am NOT sure, but here you are trying to omit entire field from the
output<br/> when its value is NULL. But that will add an extra efforts at other end<br />which is using output of this.
Thatapplication need to know all fields and<br />then need to replace NULL values for missing fields. However we have
an<br/> optional argument for ignoring nulls and thus we are safe. Application will<br />use as per its choice.<br
/><br/>Well, apart from that, tried reviewing the patch. Patch was applied but make<br />failed with following
error.<br/><br />make[3]: Entering directory `/home/jeevan/pg_master/src/backend/catalog'<br />cd
../../../src/include/catalog&& '/usr/bin/perl' ./duplicate_oids<br />3255<br />make[3]: *** [postgres.bki]
Error1<br /><br /> Please run unused_oids script to find unused oid.<br /><br />However, I had a quick look over code
changes.At first glance it looks<br />good. But still need to check on SQL level and then code walk-through.<br /><br
/>Waitingfor updated patch.<br /><br />Thanks<br /><br /><div class="gmail_extra">-- <br /><div dir="ltr">Jeevan B
Chalke<br/>Principal Software Engineer, Product Development<br />EnterpriseDB Corporation<br />The Enterprise
PostgreSQLCompany<br /><br /></div></div></div> 

<div dir="ltr">Hi Pavel,<br /><br />Patch does look good to me. And found no issues as such.<br /><br />However here
aremy optional suggestions:<br /><br />1. Frankly, I did not like name of the function "row_to_json_pretty_choosy".<br
/>Something like "row_to_json_pretty_ignore_nulls" seems better to me.<br /><br />2. To use ignore nulls feature, I
haveto always pass pretty flag.<br />Which seems weired.<br /><br />Since we do support named argument, can we avoid
that?<br/> No idea how much difficult it is. If we have a default arguments to this<br />function then we do not need
oneand two argument variations for this<br />function as well. And we can use named argument for omitting the
required<br/> one. Just a thought.<br /><br />Rest looks good to me.<br /><br />Thanks<br /><div
class="gmail_extra"><br/>-- <br /><div dir="ltr">Jeevan B Chalke<br />Principal Software Engineer, Product
Development<br/>EnterpriseDB Corporation<br />The Enterprise PostgreSQL Company<br /><br /></div></div></div> 

<div dir="ltr">Hi Pavel,<br /><br />Here are few more comments on new implementation.<br /><br />1.<br /> <span
style="font-family:couriernew,monospace">/*<br />- * SQL function row_to_json(row)<br />+ * SQL function
row_to_json(rowrecord, pretty bool, ignore_nulls bool)<br />   */<br /></span><br />In above comments, parameter name
"row"should changed to "rowval".<br /><br />2.<br /><span style="font-family:courier new,monospace">-DATA(insert OID =
3155(  row_to_json       PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 114 "2249" _null_ _null_ _null_ _null_ row_to_json
_null__null_ _null_ ));<br /> +DATA(insert OID = 3155 (  row_to_json       PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 114
"224916 16" _null_ _null_ "{rowval,pretty,ignore_nulls}" _null_ row_to_json _null_ _null_ _null_ ));<br /></span><br />
Numberof arguments (pronargs) should be 3 now. However, when we create it<br />again with default values it gets
updated.But still here we should not have<br />inconsistency.<br /><br />3.<br /><span style="font-family:courier
new,monospace"> externDatum row_to_json(PG_FUNCTION_ARGS);<br />  extern Datum row_to_json_pretty(PG_FUNCTION_ARGS);<br
/>+externDatum row_to_json_pretty_choosy(PG_FUNCTION_ARGS);<br /> extern Datum to_json(PG_FUNCTION_ARGS);<br
/></span><br/>With this new implementation, we have NOT added row_to_json_pretty_choosy()<br /> function. So need to
removethat added line. Also we have only one function<br />with default arguments and thus removed row_to_json_pretty()
functionas<br />well. Thus need to remove extern for that too.<br /><br />4.<br />Can we have couple of test cases with
namedargument along with skipped<br /> pretty parameter test?<br /><br /><br />Thanks<br /><br /><div
class="gmail_extra">--<br /><div dir="ltr">Jeevan B Chalke<br />Principal Software Engineer, Product Development<br
/>EnterpriseDBCorporation<br />The Enterprise PostgreSQL Company<br /><br /></div></div></div> 

Pavel, All,

* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> Thank you

I made a few minor adjustments, but the bigger issue (in my view) is
what to do about array_to_json_pretty- seems like we should do the same
there, no?  Perhaps you could propose a new patch which incorporates my
minor changes and also removes array_to_json_pretty and makes
array_to_json take an optional argument?

Thoughts?

    Thanks,

        Stephen

* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> ignore_nulls in array_to_json_pretty probably is not necessary. On second
> hand, the cost is zero, and we can have it for API consistency.

I'm willing to be persuaded either way on this, really.  I do think it
would be nice for both array_to_json and row_to_json to be single
functions which take default values, but as for if array_to_json has a
ignore_nulls option, I'm on the fence and would defer to people who use
that function regularly (I don't today).

Beyond that, I'm pretty happy moving forward with this patch.
Thanks,
    Stephen

* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> Can I help with something, it is there some open question?

I had been hoping for a more definitive answer regarding this option for
array_to_json, or even a comment about the change to row_to_json.
Andrew- any thoughts on this?  (that's what the ping on IRC is for :).
Thanks,
    Stephen

> 2014-09-08 6:27 GMT+02:00 Stephen Frost <sfrost@snowman.net>:
> > * Pavel Stehule (pavel.stehule@gmail.com) wrote:
> > > ignore_nulls in array_to_json_pretty probably is not necessary. On second
> > > hand, the cost is zero, and we can have it for API consistency.
> >
> > I'm willing to be persuaded either way on this, really.  I do think it
> > would be nice for both array_to_json and row_to_json to be single
> > functions which take default values, but as for if array_to_json has a
> > ignore_nulls option, I'm on the fence and would defer to people who use
> > that function regularly (I don't today).
> >
> > Beyond that, I'm pretty happy moving forward with this patch.

On 09/11/2014 08:29 AM, Stephen Frost wrote:
> * Pavel Stehule (pavel.stehule@gmail.com) wrote:
>> Can I help with something, it is there some open question?
> I had been hoping for a more definitive answer regarding this option for
> array_to_json, or even a comment about the change to row_to_json.
> Andrew- any thoughts on this?  (that's what the ping on IRC is for :).


The change in row_to_json looks OK, I think. we're replacing an 
overloading with use of default params, yes? That seems quite 
reasonable, and users shouldn't notice the difference.

There might be a case for optionally suppressing nulls in array_to_json, 
and it might work reasonably since unlike SQL arrays JSON arrays don't 
have to be regular (if nested they are arrays of arrays, not 
multi-dimensional single arrays). OTOH I'm not sure if it's worth doing 
just for the sake of orthogonality. If someone wants it, then go for it.

cheers

andrew

Andrew,

* Andrew Dunstan (andrew@dunslane.net) wrote:
> On 09/11/2014 08:29 AM, Stephen Frost wrote:
> >* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> >>Can I help with something, it is there some open question?
> >I had been hoping for a more definitive answer regarding this option for
> >array_to_json, or even a comment about the change to row_to_json.
> >Andrew- any thoughts on this?  (that's what the ping on IRC is for :).
>
> The change in row_to_json looks OK, I think. we're replacing an
> overloading with use of default params, yes? That seems quite
> reasonable, and users shouldn't notice the difference.

Right.  Great, thanks.

> There might be a case for optionally suppressing nulls in
> array_to_json, and it might work reasonably since unlike SQL arrays
> JSON arrays don't have to be regular (if nested they are arrays of
> arrays, not multi-dimensional single arrays). OTOH I'm not sure if
> it's worth doing just for the sake of orthogonality. If someone
> wants it, then go for it.

Ok.  I'll handle updating both of these to remove the overloading
and use default params instead, but I'll only add the 'ignore_null'
option to row_to_json.
Thanks again!
    Stephen

All,

* Stephen Frost (sfrost@snowman.net) wrote:
> Ok.  I'll handle updating both of these to remove the overloading
> and use default params instead, but I'll only add the 'ignore_null'
> option to row_to_json.

Barring objections or any issues I find as I go back through it, this is
what I'm planning to push a bit later on tonight.

    Thanks!

        Stephen

* Stephen Frost (sfrost@snowman.net) wrote:
> * Stephen Frost (sfrost@snowman.net) wrote:
> > Ok.  I'll handle updating both of these to remove the overloading
> > and use default params instead, but I'll only add the 'ignore_null'
> > option to row_to_json.
>
> Barring objections or any issues I find as I go back through it, this is
> what I'm planning to push a bit later on tonight.

Pushed.
Thanks!
    Stephen