Re: json_query conditional wrapper bug - Mailing list pgsql-hackers

On 05.09.24 17:01, Andrew Dunstan wrote:
> 
> On 2024-09-04 We 4:10 PM, Andrew Dunstan wrote:
>>
>> On 2024-09-04 We 6:16 AM, Peter Eisentraut wrote:
>>> On 28.08.24 11:21, Peter Eisentraut wrote:
>>>> These are ok:
>>>>
>>>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' without 
>>>> wrapper);
>>>>   json_query
>>>> ------------
>>>>   42
>>>>
>>>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with 
>>>> unconditional wrapper);
>>>>   json_query
>>>> ------------
>>>>   [42]
>>>>
>>>> But this appears to be wrong:
>>>>
>>>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with 
>>>> conditional wrapper);
>>>>   json_query
>>>> ------------
>>>>   [42]
>>>>
>>>> This should return an unwrapped 42.
>>>
>>> If I make the code change illustrated in the attached patch, then I 
>>> get the correct result here.  And various regression test results 
>>> change, which, to me, all look more correct after this patch.  I 
>>> don't know what the code I removed was supposed to accomplish, but it 
>>> seems to be wrong somehow.  In the current implementation, the WITH 
>>> CONDITIONAL WRAPPER clause doesn't appear to work correctly in any 
>>> case I could identify.
>>
>>
>> Agree the code definitely looks wrong. If anything the test should 
>> probably be reversed:
>>
>>         wrap = count > 1  || !(
>>             IsAJsonbScalar(singleton) ||
>>             (singleton->type == jbvBinary &&
>> JsonContainerIsScalar(singleton->val.binary.data)));
>>
>> i.e. in the count = 1 case wrap unless it's a scalar or a binary 
>> wrapping a scalar. The code could do with a comment about the logic.
>>
>> I know we're very close to release but we should fix this as it's a 
>> new feature.
> 
> 
> I thought about this again.
> 
> I don't know what the spec says,

Here is the relevant bit:

a) Case:
i) If the length of SEQ is 0 (zero), then let WRAPIT be False.
NOTE 479 — This ensures that the ON EMPTY behavior supersedes the 
WRAPPER behavior.
ii) If WRAPPER is WITHOUT ARRAY, then let WRAPIT be False.
iii) If WRAPPER is WITH UNCONDITIONAL ARRAY, then let WRAPIT be True.
iv) If WRAPPER is WITH CONDITIONAL ARRAY, then
Case:
1) If SEQ has a single SQL/JSON item, then let WRAPIT be False.
2) Otherwise, let WRAPIT be True.

 > but the Oracle docs say:>
>     Specify |WITH| |CONDITIONAL| |WRAPPER| to include the array wrapper
>     only if the path expression matches a single scalar value or
>     multiple values of any type. If the path expression matches a single
>     JSON object or JSON array, then the array wrapper is omitted.
> 
> So I now think the code that's there now is actually correct, and what 
> you say appears wrong is also correct.

I tested the above test expressions as well as the regression test case 
against Oracle and it agrees with my solution.  So it seems to me that 
this piece of documentation is wrong.