> On Sep 5, 2024, at 11:51 AM, Peter Eisentraut <pe...@eisentraut.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
