jsonb_concat: make sure we always return a non-scalar value

Started by Oskari Saarenmaaover 10 years ago9 messages

os@ohmu.fi

over 10 years ago

1 attachment(s)

There was a long thread about concatenating jsonb objects to each other,
but that discussion didn't touch concatenating other types. Currently
jsonb_concat always just returns the other argument if one of arguments
is considered empty. This causes surprising behavior when concatenating
scalar values to empty arrays:

os=# select '[]'::jsonb || '1'::jsonb;
1

os=# select '[]'::jsonb || '[1]os=# select '[]'::jsonb || '1'::jsonb || '2'::jsonb; [1, 2]'::jsonb;
[1]: os=# select '[]'::jsonb || '1'::jsonb || '2'::jsonb; [1, 2]

os=# select '[]'::jsonb || '1'::jsonb || '2'::jsonb;
[1, 2]

os=# select '0'::jsonb || '1'::jsonb;
[0, 1]

os=# select '{"x": "y"}'::jsonb || '[1]os=# select '[]'::jsonb || '1'::jsonb || '2'::jsonb; [1, 2]'::jsonb;
[{"x": "y"}, 1]

os=# select '{"x": "y"}'::jsonb || '1'::jsonb;
ERROR: invalid concatenation of jsonb objects

Attached a patch to fix and test this. Also added a test case for
concatenating two scalar values which currently produces an array.. I'm
not sure that behavior makes sense, but didn't want to change that in
this patch as I guess someone could consider that feature useful.

/ Oskari

Attachments:

0001-jsonb_concat-make-sure-we-always-return-a-non-scalar.patchtext/x-patch; name=0001-jsonb_concat-make-sure-we-always-return-a-non-scalar.patchDownload

>From 299124e63bb26ab07fa8429b7d1c2035b81f15d5 Mon Sep 17 00:00:00 2001
From: Oskari Saarenmaa <os@ohmu.fi>
Date: Sat, 5 Sep 2015 09:33:58 +0300
Subject: [PATCH] jsonb_concat: make sure we always return a non-scalar value

jsonb_concat used to always just return the other argument if one of
arguments was considered empty.  This caused surprising behavior when
concatenating scalar values to empty arrays.

Fixed this and added a test case for it.  Also added a test case for
concatenating two scalar values which currently produces an array.
---
 src/backend/utils/adt/jsonfuncs.c   |  8 +++++---
 src/test/regress/expected/jsonb.out | 18 ++++++++++++++++++
 src/test/regress/sql/jsonb.sql      |  4 ++++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 3b8d42e..57edb63 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -3359,11 +3359,13 @@ jsonb_concat(PG_FUNCTION_ARGS)
 			   *it2;
 
 	/*
-	 * If one of the jsonb is empty, just return other.
+	 * If one of the jsonb is empty, just return other if it's not
+	 * scalar.  If it's a scalar we need to perform concatenation to
+	 * make sure we return a non-scalar value.
 	 */
-	if (JB_ROOT_COUNT(jb1) == 0)
+	if (JB_ROOT_COUNT(jb1) == 0 && !JB_ROOT_IS_SCALAR(jb2))
 		PG_RETURN_JSONB(jb2);
-	else if (JB_ROOT_COUNT(jb2) == 0)
+	else if (JB_ROOT_COUNT(jb2) == 0 && !JB_ROOT_IS_SCALAR(jb1))
 		PG_RETURN_JSONB(jb1);
 
 	it1 = JsonbIteratorInit(&jb1->root);
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index 17656d4..98a318b 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -2912,6 +2912,24 @@ select '"c"' || '["a", "b"]'::jsonb;
  ["c", "a", "b"]
 (1 row)
 
+select '[]'::jsonb || '["a"]'::jsonb;
+ ?column? 
+----------
+ ["a"]
+(1 row)
+
+select '[]'::jsonb || '"a"'::jsonb;
+ ?column? 
+----------
+ ["a"]
+(1 row)
+
+select '"b"'::jsonb || '"a"'::jsonb;
+  ?column?  
+------------
+ ["b", "a"]
+(1 row)
+
 select '"a"'::jsonb || '{"a":1}';
 ERROR:  invalid concatenation of jsonb objects
 select '{"a":1}' || '"a"'::jsonb;
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index 83ed4eb..5a2b4a8 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -718,6 +718,10 @@ select '["c"]' || '["a", "b"]'::jsonb;
 select '["a", "b"]'::jsonb || '"c"';
 select '"c"' || '["a", "b"]'::jsonb;
 
+select '[]'::jsonb || '["a"]'::jsonb;
+select '[]'::jsonb || '"a"'::jsonb;
+select '"b"'::jsonb || '"a"'::jsonb;
+
 select '"a"'::jsonb || '{"a":1}';
 select '{"a":1}' || '"a"'::jsonb;
 
-- 
2.5.0

Andrew Dunstan

andrew@dunslane.net

over 10 years ago

In reply to: Oskari Saarenmaa (#1)

Re: jsonb_concat: make sure we always return a non-scalar value

On 09/05/2015 02:47 AM, Oskari Saarenmaa wrote:

There was a long thread about concatenating jsonb objects to each
other, but that discussion didn't touch concatenating other types.
Currently jsonb_concat always just returns the other argument if one
of arguments is considered empty. This causes surprising behavior
when concatenating scalar values to empty arrays:

os=# select '[]'::jsonb || '1'::jsonb;
1

os=# select '[]'::jsonb || '[1]'::jsonb;
[1]

os=# select '[]'::jsonb || '1'::jsonb || '2'::jsonb;
[1, 2]

os=# select '0'::jsonb || '1'::jsonb;
[0, 1]

os=# select '{"x": "y"}'::jsonb || '[1]'::jsonb;
[{"x": "y"}, 1]

os=# select '{"x": "y"}'::jsonb || '1'::jsonb;
ERROR: invalid concatenation of jsonb objects

Attached a patch to fix and test this. Also added a test case for
concatenating two scalar values which currently produces an array..
I'm not sure that behavior makes sense, but didn't want to change that
in this patch as I guess someone could consider that feature useful.

This looks correct. Barring objection I'll apply this shortly.

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

Teodor Sigaev

teodor@sigaev.ru

over 10 years ago

In reply to: Andrew Dunstan (#2)

Re: jsonb_concat: make sure we always return a non-scalar value

This looks correct. Barring objection I'll apply this shortly.

+1 Seems correct. Should we backpatch that?

--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Dunstan

andrew@dunslane.net

over 10 years ago

In reply to: Teodor Sigaev (#3)

Re: jsonb_concat: make sure we always return a non-scalar value

On 09/08/2015 10:48 AM, Teodor Sigaev wrote:

This looks correct. Barring objection I'll apply this shortly.

+1 Seems correct. Should we backpatch that?

Yes, certainly.

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

Thom Brown

thom@linux.com

over 10 years ago

In reply to: Teodor Sigaev (#3)

Re: jsonb_concat: make sure we always return a non-scalar value

On 8 September 2015 at 15:48, Teodor Sigaev <teodor@sigaev.ru> wrote:

This looks correct. Barring objection I'll apply this shortly.

+1 Seems correct. Should we backpatch that?

Yes, this needs correcting in 9.5 where it has been introduced.

Thom

Andrew Dunstan

andrew@dunslane.net

over 10 years ago

In reply to: Andrew Dunstan (#2)

Re: jsonb_concat: make sure we always return a non-scalar value

On 09/08/2015 09:54 AM, Andrew Dunstan wrote:

On 09/05/2015 02:47 AM, Oskari Saarenmaa wrote:

There was a long thread about concatenating jsonb objects to each
other, but that discussion didn't touch concatenating other types.
Currently jsonb_concat always just returns the other argument if one
of arguments is considered empty. This causes surprising behavior
when concatenating scalar values to empty arrays:

os=# select '[]'::jsonb || '1'::jsonb;
1

os=# select '[]'::jsonb || '[1]'::jsonb;
[1]

os=# select '[]'::jsonb || '1'::jsonb || '2'::jsonb;
[1, 2]

os=# select '0'::jsonb || '1'::jsonb;
[0, 1]

os=# select '{"x": "y"}'::jsonb || '[1]'::jsonb;
[{"x": "y"}, 1]

os=# select '{"x": "y"}'::jsonb || '1'::jsonb;
ERROR: invalid concatenation of jsonb objects

Attached a patch to fix and test this. Also added a test case for
concatenating two scalar values which currently produces an array..
I'm not sure that behavior makes sense, but didn't want to change
that in this patch as I guess someone could consider that feature
useful.

This looks correct. Barring objection I'll apply this shortly.

Actually, I'm not sure the test is sufficient here. It looks to me like
we should only be taking this fast path if one operand is an empty array
and the other is a non-scalar array.

Otherwise we get things like this (second case is wrong, I think):

andrew=# select '[]'::jsonb || '"a"';
?column?
----------
["a"]

andrew=# select '[]'::jsonb || '{"a":"b"}';
?column?
------------
{"a": "b"}

andrew=# select '[1]'::jsonb || '{"a":"b"}';
?column?
-----------------
[1, {"a": "b"}]

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

Oskari Saarenmaa

os@ohmu.fi

over 10 years ago

In reply to: Andrew Dunstan (#6)

Re: jsonb_concat: make sure we always return a non-scalar value

09.09.2015, 18:53, Andrew Dunstan kirjoitti:

On 09/08/2015 09:54 AM, Andrew Dunstan wrote:

On 09/05/2015 02:47 AM, Oskari Saarenmaa wrote:

There was a long thread about concatenating jsonb objects to each
other, but that discussion didn't touch concatenating other types.
Currently jsonb_concat always just returns the other argument if one
of arguments is considered empty. This causes surprising behavior
when concatenating scalar values to empty arrays:

os=# select '[]'::jsonb || '1'::jsonb;
1

os=# select '[]'::jsonb || '[1]'::jsonb;
[1]

os=# select '[]'::jsonb || '1'::jsonb || '2'::jsonb;
[1, 2]

os=# select '0'::jsonb || '1'::jsonb;
[0, 1]

os=# select '{"x": "y"}'::jsonb || '[1]'::jsonb;
[{"x": "y"}, 1]

os=# select '{"x": "y"}'::jsonb || '1'::jsonb;
ERROR: invalid concatenation of jsonb objects

Attached a patch to fix and test this. Also added a test case for
concatenating two scalar values which currently produces an array..
I'm not sure that behavior makes sense, but didn't want to change
that in this patch as I guess someone could consider that feature
useful.

This looks correct. Barring objection I'll apply this shortly.

Actually, I'm not sure the test is sufficient here. It looks to me like
we should only be taking this fast path if one operand is an empty array
and the other is a non-scalar array.

Otherwise we get things like this (second case is wrong, I think):

andrew=# select '[]'::jsonb || '"a"';
?column?
----------
["a"]

andrew=# select '[]'::jsonb || '{"a":"b"}';
?column?
------------
{"a": "b"}

andrew=# select '[1]'::jsonb || '{"a":"b"}';
?column?
-----------------
[1, {"a": "b"}]

It looks wrong, but I'm not sure what's right in that case. I think
it'd make sense to just restrict concatenation to object || object,
array || array and array || scalar and document that. I doubt many
people expect their objects to turn into arrays if they accidentally
concatenate an array into it. Alternatively the behavior could depend
on the order of arguments for concatenation, array || anything -> array,
object || array -> error.

/ Oskari

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Jim Nasby

Jim.Nasby@BlueTreble.com

over 10 years ago

In reply to: Oskari Saarenmaa (#7)

Re: jsonb_concat: make sure we always return a non-scalar value

On 9/9/15 12:03 PM, Oskari Saarenmaa wrote:

andrew=# select '[1]'::jsonb || '{"a":"b"}';
?column?
-----------------
[1, {"a": "b"}]

It looks wrong, but I'm not sure what's right in that case. I think
it'd make sense to just restrict concatenation to object || object,
array || array and array || scalar and document that. I doubt many
people expect their objects to turn into arrays if they accidentally
concatenate an array into it. Alternatively the behavior could depend
on the order of arguments for concatenation, array || anything -> array,
object || array -> error.

That definitely doesn't sound like a good default.

It might be useful to have a concat function that would concatinate
anything into an array. But if we don't provide one by default users
could always create their own with json__typeof() and json_build_array().
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Dunstan

andrew@dunslane.net

over 10 years ago

In reply to: Oskari Saarenmaa (#7)

1 attachment(s)

Re: jsonb_concat: make sure we always return a non-scalar value

On 09/09/2015 01:03 PM, Oskari Saarenmaa wrote:

09.09.2015, 18:53, Andrew Dunstan kirjoitti:

On 09/08/2015 09:54 AM, Andrew Dunstan wrote:

On 09/05/2015 02:47 AM, Oskari Saarenmaa wrote:

There was a long thread about concatenating jsonb objects to each
other, but that discussion didn't touch concatenating other types.
Currently jsonb_concat always just returns the other argument if one
of arguments is considered empty. This causes surprising behavior
when concatenating scalar values to empty arrays:

os=# select '[]'::jsonb || '1'::jsonb;
1

os=# select '[]'::jsonb || '[1]'::jsonb;
[1]

os=# select '[]'::jsonb || '1'::jsonb || '2'::jsonb;
[1, 2]

os=# select '0'::jsonb || '1'::jsonb;
[0, 1]

os=# select '{"x": "y"}'::jsonb || '[1]'::jsonb;
[{"x": "y"}, 1]

os=# select '{"x": "y"}'::jsonb || '1'::jsonb;
ERROR: invalid concatenation of jsonb objects

Attached a patch to fix and test this. Also added a test case for
concatenating two scalar values which currently produces an array..
I'm not sure that behavior makes sense, but didn't want to change
that in this patch as I guess someone could consider that feature
useful.

This looks correct. Barring objection I'll apply this shortly.

Actually, I'm not sure the test is sufficient here. It looks to me like
we should only be taking this fast path if one operand is an empty array
and the other is a non-scalar array.

Otherwise we get things like this (second case is wrong, I think):

andrew=# select '[]'::jsonb || '"a"';
?column?
----------
["a"]

andrew=# select '[]'::jsonb || '{"a":"b"}';
?column?
------------
{"a": "b"}

andrew=# select '[1]'::jsonb || '{"a":"b"}';
?column?
-----------------
[1, {"a": "b"}]

It looks wrong, but I'm not sure what's right in that case. I think
it'd make sense to just restrict concatenation to object || object,
array || array and array || scalar and document that. I doubt many
people expect their objects to turn into arrays if they accidentally
concatenate an array into it. Alternatively the behavior could depend
on the order of arguments for concatenation, array || anything ->
array, object || array -> error.

I don't think I want to change the semantics in general at this stage. A
simple minimal fix to handle the case of the fastpath where one of other
operand is empty, making it entirely consistent with the case where it's
not empty, is attached.

cheers

andrew

Attachments:

jsonbconcatfix.patchapplication/x-patch; name=jsonbconcatfix.patchDownload

diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 3b8d42e..154a883 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -3359,12 +3359,18 @@ jsonb_concat(PG_FUNCTION_ARGS)
 			   *it2;
 
 	/*
-	 * If one of the jsonb is empty, just return other.
+	 * If one of the jsonb is empty, just return the other if it's not
+	 * scalar and both are of the same kind.  If it's a scalar or they are
+	 * of different kinds we need to perform the concatenation even if one is
+	 * empty.
 	 */
-	if (JB_ROOT_COUNT(jb1) == 0)
-		PG_RETURN_JSONB(jb2);
-	else if (JB_ROOT_COUNT(jb2) == 0)
-		PG_RETURN_JSONB(jb1);
+	if (JB_ROOT_IS_OBJECT(jb1) == JB_ROOT_IS_OBJECT(jb2))
+	{
+		if (JB_ROOT_COUNT(jb1) == 0 && !JB_ROOT_IS_SCALAR(jb2))
+			PG_RETURN_JSONB(jb2);
+		else if (JB_ROOT_COUNT(jb2) == 0 && !JB_ROOT_IS_SCALAR(jb1))
+			PG_RETURN_JSONB(jb1);
+	}
 
 	it1 = JsonbIteratorInit(&jb1->root);
 	it2 = JsonbIteratorInit(&jb2->root);
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index 17656d4..82d1b69 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -2912,6 +2912,42 @@ select '"c"' || '["a", "b"]'::jsonb;
  ["c", "a", "b"]
 (1 row)
 
+select '[]'::jsonb || '["a"]'::jsonb;
+ ?column? 
+----------
+ ["a"]
+(1 row)
+
+select '[]'::jsonb || '"a"'::jsonb;
+ ?column? 
+----------
+ ["a"]
+(1 row)
+
+select '"b"'::jsonb || '"a"'::jsonb;
+  ?column?  
+------------
+ ["b", "a"]
+(1 row)
+
+select '{}'::jsonb || '{"a":"b"}'::jsonb;
+  ?column?  
+------------
+ {"a": "b"}
+(1 row)
+
+select '[]'::jsonb || '{"a":"b"}'::jsonb;
+   ?column?   
+--------------
+ [{"a": "b"}]
+(1 row)
+
+select '{"a":"b"}'::jsonb || '[]'::jsonb;
+   ?column?   
+--------------
+ [{"a": "b"}]
+(1 row)
+
 select '"a"'::jsonb || '{"a":1}';
 ERROR:  invalid concatenation of jsonb objects
 select '{"a":1}' || '"a"'::jsonb;
diff --git a/src/test/regress/expected/jsonb_1.out b/src/test/regress/expected/jsonb_1.out
index 86b1162..2cbc0c7 100644
--- a/src/test/regress/expected/jsonb_1.out
+++ b/src/test/regress/expected/jsonb_1.out
@@ -2912,6 +2912,42 @@ select '"c"' || '["a", "b"]'::jsonb;
  ["c", "a", "b"]
 (1 row)
 
+select '[]'::jsonb || '["a"]'::jsonb;
+ ?column? 
+----------
+ ["a"]
+(1 row)
+
+select '[]'::jsonb || '"a"'::jsonb;
+ ?column? 
+----------
+ ["a"]
+(1 row)
+
+select '"b"'::jsonb || '"a"'::jsonb;
+  ?column?  
+------------
+ ["b", "a"]
+(1 row)
+
+select '{}'::jsonb || '{"a":"b"}'::jsonb;
+  ?column?  
+------------
+ {"a": "b"}
+(1 row)
+
+select '[]'::jsonb || '{"a":"b"}'::jsonb;
+   ?column?   
+--------------
+ [{"a": "b"}]
+(1 row)
+
+select '{"a":"b"}'::jsonb || '[]'::jsonb;
+   ?column?   
+--------------
+ [{"a": "b"}]
+(1 row)
+
 select '"a"'::jsonb || '{"a":1}';
 ERROR:  invalid concatenation of jsonb objects
 select '{"a":1}' || '"a"'::jsonb;
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index 83ed4eb..cb03ada 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -718,6 +718,13 @@ select '["c"]' || '["a", "b"]'::jsonb;
 select '["a", "b"]'::jsonb || '"c"';
 select '"c"' || '["a", "b"]'::jsonb;
 
+select '[]'::jsonb || '["a"]'::jsonb;
+select '[]'::jsonb || '"a"'::jsonb;
+select '"b"'::jsonb || '"a"'::jsonb;
+select '{}'::jsonb || '{"a":"b"}'::jsonb;
+select '[]'::jsonb || '{"a":"b"}'::jsonb;
+select '{"a":"b"}'::jsonb || '[]'::jsonb;
+
 select '"a"'::jsonb || '{"a":1}';
 select '{"a":1}' || '"a"'::jsonb;