Обсуждение: jsonb_concat: make sure we always return a non-scalar value

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.

/ Oskari

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

> 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/
 

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

<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 8 September 2015 at 15:48, Teodor Sigaev <span
dir="ltr"><<ahref="mailto:teodor@sigaev.ru" target="_blank">teodor@sigaev.ru</a>></span> wrote:<br /><blockquote
class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><blockquote
class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> This looks correct. Barring
objectionI'll apply this shortly.<br /></blockquote><br /></span> +1 Seems correct. Should we backpatch that?<span
class="HOEnZb"><fontcolor="#888888"><br /></font></span></blockquote><br />Yes, this needs correcting in 9.5 where it
hasbeen introduced.<br /></div><br /><div class="gmail_signature">Thom</div></div></div> 

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

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

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

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