Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kqiww-0003XE-47 for pgsql-hackers@arkaria.postgresql.org; Sat, 19 Dec 2020 20:35:34 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1kqiwu-0005WH-Km for pgsql-hackers@arkaria.postgresql.org; Sat, 19 Dec 2020 20:35:32 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kqiwu-0005W7-9j for pgsql-hackers@lists.postgresql.org; Sat, 19 Dec 2020 20:35:32 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kqiws-0001Xl-0B for pgsql-hackers@lists.postgresql.org; Sat, 19 Dec 2020 20:35:31 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 0BJKZS3O333853 for ; Sat, 19 Dec 2020 15:35:28 -0500 From: Tom Lane To: pgsql-hackers@lists.postgresql.org Subject: Re: Weird special case in jsonb_concat() In-reply-to: <163099.1608312033@sss.pgh.pa.us> References: <163099.1608312033@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Fri, 18 Dec 2020 12:20:33 -0500" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <333716.1608409963.0@sss.pgh.pa.us> Date: Sat, 19 Dec 2020 15:35:28 -0500 Message-ID: <333852.1608410128@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <333716.1608409963.1@sss.pgh.pa.us> I wrote: > However, further experimentation found a case that fails: > regression=# select '3'::jsonb || '{}'::jsonb; > ERROR: invalid concatenation of jsonb objects > I wonder what is the point of this weird exception, and whether > whoever devised it can provide a concise explanation of what > they think the full behavior of "jsonb || jsonb" is. Why isn't > '[3, {}]' a reasonable result here, if the cases above are OK? Here is a proposed patch for that. It turns out that the third else-branch in IteratorConcat() already does the right thing, if we just remove its restrictive else-condition and let it handle everything except the two-objects and two-arrays cases. But it seemed to me that trying to handle both the object || array and array || object cases in that one else-branch was poorly thought out: only one line of code can actually be shared, and it took several extra lines of infrastructure to support the sharing. So I split those cases into separate else-branches. This also addresses the inadequate documentation that was the original complaint. Thoughts? Should we back-patch this? The existing behavior seems to me to be inconsistent enough to be arguably a bug, but we've not had field complaints saying "this should work". regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-jsonb-concatenation-special-cases.patch"; charset="us-ascii" Content-ID: <333716.1608409963.2@sss.pgh.pa.us> Content-Description: fix-jsonb-concatenation-special-cases.patch Content-Transfer-Encoding: quoted-printable diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index df29af6371..5f3c393a25 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -14715,8 +14715,12 @@ table2-mapping Concatenates two jsonb values. - Concatenating two objects generates an object with the union of t= heir + Concatenating two arrays generates an array containing all the + elements of each input. Concatenating two objects generates an + object containing the union of their keys, taking the second object's value when there are duplicate k= eys. + All other cases are treated by converting a non-array input into = a + single-element array, and then proceeding as for two arrays. Does not operate recursively: only the top-level array or object structure is merged. @@ -14727,6 +14731,14 @@ table2-mapping '{"a": "b"}'::jsonb || '{"c": "d"}'::jsonb {"a": "b", "c": "d"} + + + '[1, 2]'::jsonb || '3'::jsonb + [1, 2, 3] + + + '{"a": "b"}'::jsonb || '42'::jsonb + [{"a": "b"}, 42] = diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jso= nfuncs.c index 7a25415078..5beade00f4 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -4690,11 +4690,14 @@ IteratorConcat(JsonbIterator **it1, JsonbIterator = **it2, rk2 =3D JsonbIteratorNext(it2, &v2, false); = /* - * Both elements are objects. + * JsonbIteratorNext reports raw scalars as if they were single-element + * arrays; hence we only need consider "object" and "array" cases here. */ if (rk1 =3D=3D WJB_BEGIN_OBJECT && rk2 =3D=3D WJB_BEGIN_OBJECT) { /* + * Both elements are objects. + * * Append all the tokens from v1 to res, except last WJB_END_OBJECT * (because res will not be finished yet). */ @@ -4703,18 +4706,18 @@ IteratorConcat(JsonbIterator **it1, JsonbIterator = **it2, pushJsonbValue(state, r1, &v1); = /* - * Append all the tokens from v2 to res, include last WJB_END_OBJECT - * (the concatenation will be completed). + * Append all the tokens from v2 to res, including last WJB_END_OBJECT + * (the concatenation will be completed). Any duplicate keys will + * automatically override the value from the first object. */ while ((r2 =3D JsonbIteratorNext(it2, &v2, true)) !=3D WJB_DONE) res =3D pushJsonbValue(state, r2, r2 !=3D WJB_END_OBJECT ? &v2 : NULL)= ; } - - /* - * Both elements are arrays (either can be scalar). - */ else if (rk1 =3D=3D WJB_BEGIN_ARRAY && rk2 =3D=3D WJB_BEGIN_ARRAY) { + /* + * Both elements are arrays. + */ pushJsonbValue(state, rk1, NULL); = while ((r1 =3D JsonbIteratorNext(it1, &v1, true)) !=3D WJB_END_ARRAY) @@ -4731,46 +4734,40 @@ IteratorConcat(JsonbIterator **it1, JsonbIterator = **it2, = res =3D pushJsonbValue(state, WJB_END_ARRAY, NULL /* signal to sort */ = ); } - /* have we got array || object or object || array? */ - else if (((rk1 =3D=3D WJB_BEGIN_ARRAY && !(*it1)->isScalar) && rk2 =3D=3D= WJB_BEGIN_OBJECT) || - (rk1 =3D=3D WJB_BEGIN_OBJECT && (rk2 =3D=3D WJB_BEGIN_ARRAY && !(*it2= )->isScalar))) + else if (rk1 =3D=3D WJB_BEGIN_OBJECT) { - JsonbIterator **it_array =3D rk1 =3D=3D WJB_BEGIN_ARRAY ? it1 : it2; - JsonbIterator **it_object =3D rk1 =3D=3D WJB_BEGIN_OBJECT ? it1 : it2; - bool prepend =3D (rk1 =3D=3D WJB_BEGIN_OBJECT); + /* + * We have object || array. + */ + Assert(rk2 =3D=3D WJB_BEGIN_ARRAY); = pushJsonbValue(state, WJB_BEGIN_ARRAY, NULL); = - if (prepend) - { - pushJsonbValue(state, WJB_BEGIN_OBJECT, NULL); - while ((r1 =3D JsonbIteratorNext(it_object, &v1, true)) !=3D WJB_DONE) - pushJsonbValue(state, r1, r1 !=3D WJB_END_OBJECT ? &v1 : NULL); - - while ((r2 =3D JsonbIteratorNext(it_array, &v2, true)) !=3D WJB_DONE) - res =3D pushJsonbValue(state, r2, r2 !=3D WJB_END_ARRAY ? &v2 : NULL)= ; - } - else - { - while ((r1 =3D JsonbIteratorNext(it_array, &v1, true)) !=3D WJB_END_AR= RAY) - pushJsonbValue(state, r1, &v1); - - pushJsonbValue(state, WJB_BEGIN_OBJECT, NULL); - while ((r2 =3D JsonbIteratorNext(it_object, &v2, true)) !=3D WJB_DONE) - pushJsonbValue(state, r2, r2 !=3D WJB_END_OBJECT ? &v2 : NULL); + pushJsonbValue(state, WJB_BEGIN_OBJECT, NULL); + while ((r1 =3D JsonbIteratorNext(it1, &v1, true)) !=3D WJB_DONE) + pushJsonbValue(state, r1, r1 !=3D WJB_END_OBJECT ? &v1 : NULL); = - res =3D pushJsonbValue(state, WJB_END_ARRAY, NULL); - } + while ((r2 =3D JsonbIteratorNext(it2, &v2, true)) !=3D WJB_DONE) + res =3D pushJsonbValue(state, r2, r2 !=3D WJB_END_ARRAY ? &v2 : NULL); } else { /* - * This must be scalar || object or object || scalar, as that's all - * that's left. Both of these make no sense, so error out. + * We have array || object. */ - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("invalid concatenation of jsonb objects"))); + Assert(rk1 =3D=3D WJB_BEGIN_ARRAY); + Assert(rk2 =3D=3D WJB_BEGIN_OBJECT); + + pushJsonbValue(state, WJB_BEGIN_ARRAY, NULL); + + while ((r1 =3D JsonbIteratorNext(it1, &v1, true)) !=3D WJB_END_ARRAY) + pushJsonbValue(state, r1, &v1); + + pushJsonbValue(state, WJB_BEGIN_OBJECT, NULL); + while ((r2 =3D JsonbIteratorNext(it2, &v2, true)) !=3D WJB_DONE) + pushJsonbValue(state, r2, r2 !=3D WJB_END_OBJECT ? &v2 : NULL); + + res =3D pushJsonbValue(state, WJB_END_ARRAY, NULL); } = return res; diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expect= ed/jsonb.out index a70cd0b7c1..1e6c6ef200 100644 --- a/src/test/regress/expected/jsonb.out +++ b/src/test/regress/expected/jsonb.out @@ -4111,9 +4111,41 @@ select '{"a":"b"}'::jsonb || '[]'::jsonb; (1 row) = select '"a"'::jsonb || '{"a":1}'; -ERROR: invalid concatenation of jsonb objects + ?column? = +----------------- + ["a", {"a": 1}] +(1 row) + select '{"a":1}' || '"a"'::jsonb; -ERROR: invalid concatenation of jsonb objects + ?column? = +----------------- + [{"a": 1}, "a"] +(1 row) + +select '[3]'::jsonb || '{}'::jsonb; + ?column? = +---------- + [3, {}] +(1 row) + +select '3'::jsonb || '[]'::jsonb; + ?column? = +---------- + [3] +(1 row) + +select '3'::jsonb || '4'::jsonb; + ?column? = +---------- + [3, 4] +(1 row) + +select '3'::jsonb || '{}'::jsonb; + ?column? = +---------- + [3, {}] +(1 row) + select '["a", "b"]'::jsonb || '{"c":1}'; ?column? = ---------------------- diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.s= ql index 3e2b8f66df..b6409767f6 100644 --- a/src/test/regress/sql/jsonb.sql +++ b/src/test/regress/sql/jsonb.sql @@ -1056,6 +1056,11 @@ select '{"a":"b"}'::jsonb || '[]'::jsonb; select '"a"'::jsonb || '{"a":1}'; select '{"a":1}' || '"a"'::jsonb; = +select '[3]'::jsonb || '{}'::jsonb; +select '3'::jsonb || '[]'::jsonb; +select '3'::jsonb || '4'::jsonb; +select '3'::jsonb || '{}'::jsonb; + select '["a", "b"]'::jsonb || '{"c":1}'; select '{"c": 1}'::jsonb || '["a", "b"]'; = ------- =_aaaaaaaaaa0--