proposal: ignore null fields in not relation type composite type based constructors

Hello

I am sending small patch, that allows ignore nulls in row_to_json function.
It allow significant size reduction of generated JSON documents.

Regards

Pavel

2014-05-25 7:53 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hi Pavel,

You have said that XMLFOREST has something which ignores nulls, what's that?
Will you please provide an example ?

I am NOT sure, but here you are trying to omit entire field from the output
when its value is NULL. But that will add an extra efforts at other end
which is using output of this. That application need to know all fields and
then need to replace NULL values for missing fields. However we have an
optional argument for ignoring nulls and thus we are safe. Application will
use as per its choice.

Well, apart from that, tried reviewing the patch. Patch was applied but make
failed with following error.

make[3]: Entering directory `/home/jeevan/pg_master/src/backend/catalog'
cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
3255
make[3]: *** [postgres.bki] Error 1

Please run unused_oids script to find unused oid.

However, I had a quick look over code changes. At first glance it looks
good. But still need to check on SQL level and then code walk-through.

Waiting for updated patch.

Thanks

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Hello

2014-08-22 12:21 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

Hi Pavel,

You have said that XMLFOREST has something which ignores nulls, what's
that?
Will you please provide an example ?

I was partially wrong - XMLFOREST ignore null always

postgres=# select xmlforest(10 as a,20 as b,null as c);
xmlforest
--------------------
<a>10</a><b>20</b>
(1 row)

so if you would to empty elements, you should to use xmlelement and
xmlconcat

postgres=# select xmlconcat(xmlforest(10 as a,20 as b), xmlelement(name c,
null));
xmlconcat
------------------------
<a>10</a><b>20</b><c/>
(1 row)

I am NOT sure, but here you are trying to omit entire field from the output
when its value is NULL. But that will add an extra efforts at other end
which is using output of this. That application need to know all fields and
then need to replace NULL values for missing fields. However we have an
optional argument for ignoring nulls and thus we are safe. Application will
use as per its choice.

with my patch, you can do decision - lot of REST services doesn't
distinguishes between empty and missing tag - and some developers prefer
remove empty tags due less size of message.

Well, apart from that, tried reviewing the patch. Patch was applied but
make
failed with following error.

make[3]: Entering directory `/home/jeevan/pg_master/src/backend/catalog'
cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
3255
make[3]: *** [postgres.bki] Error 1

Please run unused_oids script to find unused oid.

it needs remastering

update in attachemnt

However, I had a quick look over code changes. At first glance it looks
good. But still need to check on SQL level and then code walk-through.

Waiting for updated patch.

thank you for review

Regards

Pavel

Hi Pavel,

Patch does look good to me. And found no issues as such.

However here are my optional suggestions:

1. Frankly, I did not like name of the function "row_to_json_pretty_choosy".
Something like "row_to_json_pretty_ignore_nulls" seems better to me.

2. To use ignore nulls feature, I have to always pass pretty flag.
Which seems weired.

Since we do support named argument, can we avoid that?
No idea how much difficult it is. If we have a default arguments to this
function then we do not need one and two argument variations for this
function as well. And we can use named argument for omitting the required
one. Just a thought.

Rest looks good to me.

Thanks

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

2014-09-01 12:33 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

Hi Pavel,

Patch does look good to me. And found no issues as such.

However here are my optional suggestions:

1. Frankly, I did not like name of the function
"row_to_json_pretty_choosy".
Something like "row_to_json_pretty_ignore_nulls" seems better to me.

should be - I have no better name

2. To use ignore nulls feature, I have to always pass pretty flag.
Which seems weired.

Since we do support named argument, can we avoid that?
No idea how much difficult it is. If we have a default arguments to this
function then we do not need one and two argument variations for this
function as well. And we can use named argument for omitting the required
one. Just a thought.

it needs a redesign of original implementation, we should to change API to
use default values with named parameters

but it doesn't help too much (although it can be readable little bit more)

instead row_to_json(x, false, true)

be

row_ro_json(x, ignore_null := true)

it is not too much work, but I need a names for parameters

Regards

Pavel

Hi Pavel,

it needs a redesign of original implementation, we should to change API to

use default values with named parameters

but it doesn't help too much (although it can be readable little bit more)

instead row_to_json(x, false, true)

be

row_ro_json(x, ignore_null := true)

it is not too much work, but I need a names for parameters

I have tried adding dummy names (a, b, c) in pg_proc entry you have added.
But that is not sufficient. We need to have default values provided to these
arguments to work row_ro_json(x, ignore_null := true) call.
It was not trivial. So I have not put much thought on that.

For name, I choose (row, pretty, ignore_nulls) or similar.

However it was my thought.
If it is too complex of not so useful then we can ignore it.

Thanks
--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Hello

2014-09-02 13:54 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

Hi Pavel,

it needs a redesign of original implementation, we should to change API

to use default values with named parameters

but it doesn't help too much (although it can be readable little bit more)

instead row_to_json(x, false, true)

be

row_ro_json(x, ignore_null := true)

it is not too much work, but I need a names for parameters

I have tried adding dummy names (a, b, c) in pg_proc entry you have added.
But that is not sufficient. We need to have default values provided to
these
arguments to work row_ro_json(x, ignore_null := true) call.
It was not trivial. So I have not put much thought on that.

For name, I choose (row, pretty, ignore_nulls) or similar.

However it was my thought.
If it is too complex of not so useful then we can ignore it.

here is patch

Regards

Pavel

2014-09-03 7:05 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hello

2014-09-02 13:54 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

Hi Pavel,

it needs a redesign of original implementation, we should to change API

to use default values with named parameters

but it doesn't help too much (although it can be readable little bit
more)

instead row_to_json(x, false, true)

be

row_ro_json(x, ignore_null := true)

it is not too much work, but I need a names for parameters

I have tried adding dummy names (a, b, c) in pg_proc entry you have added.
But that is not sufficient. We need to have default values provided to
these
arguments to work row_ro_json(x, ignore_null := true) call.
It was not trivial. So I have not put much thought on that.

For name, I choose (row, pretty, ignore_nulls) or similar.

I cannot use "row" because it is keyword - I used "rowval"

Regards

Pavel

Hi Pavel,

Here are few more comments on new implementation.

1.
 /*
- * SQL function row_to_json(row)
+ * SQL function row_to_json(row record, pretty bool, ignore_nulls bool)
  */

In above comments, parameter name "row" should changed to "rowval".

2.
-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_ ));
+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 16 16" _null_ _null_ "{rowval,pretty,ignore_nulls}"
_null_ row_to_json _null_ _null_ _null_ ));

Number of arguments (pronargs) should be 3 now. However, when we create it
again with default values it gets updated. But still here we should not have
inconsistency.

3.
extern Datum row_to_json(PG_FUNCTION_ARGS);
extern Datum row_to_json_pretty(PG_FUNCTION_ARGS);
+extern Datum row_to_json_pretty_choosy(PG_FUNCTION_ARGS);
extern Datum to_json(PG_FUNCTION_ARGS);

With this new implementation, we have NOT added row_to_json_pretty_choosy()
function. So need to remove that added line. Also we have only one function
with default arguments and thus removed row_to_json_pretty() function as
well. Thus need to remove extern for that too.

4.
Can we have couple of test cases with named argument along with skipped
pretty parameter test?

Thanks

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Hi

2014-09-03 9:27 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

Hi Pavel,

Here are few more comments on new implementation.

1.
/*
- * SQL function row_to_json(row)
+ * SQL function row_to_json(row record, pretty bool, ignore_nulls bool)
*/

In above comments, parameter name "row" should changed to "rowval".

2.
-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_ ));
+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 16 16" _null_ _null_ "{rowval,pretty,ignore_nulls}"
_null_ row_to_json _null_ _null_ _null_ ));

Number of arguments (pronargs) should be 3 now. However, when we create it
again with default values it gets updated. But still here we should not
have
inconsistency.

3.
extern Datum row_to_json(PG_FUNCTION_ARGS);
extern Datum row_to_json_pretty(PG_FUNCTION_ARGS);
+extern Datum row_to_json_pretty_choosy(PG_FUNCTION_ARGS);
extern Datum to_json(PG_FUNCTION_ARGS);

With this new implementation, we have NOT added row_to_json_pretty_choosy()
function. So need to remove that added line. Also we have only one function
with default arguments and thus removed row_to_json_pretty() function as
well. Thus need to remove extern for that too.

4.
Can we have couple of test cases with named argument along with skipped
pretty parameter test?

done

Regards

Pavel

Hi Pavel,

You have attached wrong patch.

Thanks
--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

I am sory

too much patches

Regards

Pavel

2014-09-04 7:35 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

On Thu, Sep 4, 2014 at 11:41 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

I am sory

too much patches

:)

Patch looks good to me.
Marking "Ready for Committer".

Thanks

Regards

Pavel

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Thank you

Regards

Pavel

2014-09-05 8:29 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

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

Hi

2014-09-08 5:48 GMT+02:00 Stephen Frost <sfrost@snowman.net>:

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?

I though about it, and I am not sure. If somebody uses arrays as set, then
it can be good idea, when it is used as array, then you cannot to throw a
nulls from array.

I am thinking, so it is not necessary, because we can remove NULLs from
array simply via iteration over array (what is impossible for row fields)

CREATE OR REPLACE FUNCTION remove_null(anyarray)
RETURNS anyarray AS $$
SELECT ARRAY(SELECT a FROM unnest($1) g(a) WHERE a IS NOT NULL)
$$ LANGUAGE plpgsql;

or this function can be in core for general usage.

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.

Regards

Pavel

* 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

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.

ok

Regards

Pavel

Hi Stephen

Can I help with something, it is there some open question?

Regards

Pavel

2014-09-08 6:27 GMT+02:00 Stephen Frost <sfrost@snowman.net>: