Re: json_query conditional wrapper bug

> On Sep 5, 2024, at 11:51 AM, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> 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.

Oh, odd. Then assuming a scalar is an SQL/JSON item your patch appears correct.

Cheers

Andrew