Thread: json_query conditional wrapper bug
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.
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.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
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, but the Oracle docs say:
Specify
WITHCONDITIONALWRAPPERto 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.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
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.
> On Sep 5, 2024, at 11:51 AM, Peter Eisentraut <peter@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
regressiontest results change, which, to me, all look more correct after this patch. I don't know what the code I
removedwas supposed to accomplish, but it seems to be wrong somehow. In the current implementation, the WITH
CONDITIONALWRAPPER 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
commentabout 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
On 10.09.24 10:00, Amit Langote wrote:
> Sorry for missing this report and thanks Andrew for the offlist heads up.
>
> On Wed, Sep 4, 2024 at 7:16 PM Peter Eisentraut <peter@eisentraut.org> 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.
>
> Agreed that this looks wrong.
>
> I've wondered why the condition was like that but left it as-is,
> because I thought at one point that that's needed to ensure that the
> returned single scalar SQL/JSON item is valid jsonb.
>
> I've updated your patch to include updated test outputs and a nearby
> code comment expanded. Do you intend to commit it or do you prefer
> that I do?
This change looks unrelated:
-ERROR: new row for relation "test_jsonb_constraints" violates check
constraint "test_jsonb_constraint4"
+ERROR: new row for relation "test_jsonb_constraints" violates check
constraint "test_jsonb_constraint5"
Is this some randomness in the way these constraints are evaluated?
On 11.09.24 09:51, Amit Langote wrote: >>> I've updated your patch to include updated test outputs and a nearby >>> code comment expanded. Do you intend to commit it or do you prefer >>> that I do? >> >> This change looks unrelated: >> >> -ERROR: new row for relation "test_jsonb_constraints" violates check >> constraint "test_jsonb_constraint4" >> +ERROR: new row for relation "test_jsonb_constraints" violates check >> constraint "test_jsonb_constraint5" >> >> Is this some randomness in the way these constraints are evaluated? > > The result of JSON_QUERY() in the CHECK constraint changes, so the > constraint that previously failed now succeeds after this change, > because the comparison looked like this before and after: > > -- before > postgres=# select jsonb '[10]' < jsonb '[10]'; > ?column? > ---------- > f > (1 row) > > -- after > postgres=# select jsonb '10' < jsonb '[10]'; > ?column? > ---------- > t > (1 row) > > That causes the next constraint to be evaluated and its failure > reported instead. > > In the attached, I've adjusted the constraint for the test case to be > a bit more relevant and removed a nearby somewhat redundant test, > mainly because its output changes after the adjustment. Ok, that looks good. Good that we could clear that up a bit.
On Wed, Sep 11, 2024 at 6:57 PM Peter Eisentraut <peter@eisentraut.org> wrote: > On 11.09.24 09:51, Amit Langote wrote: > >>> I've updated your patch to include updated test outputs and a nearby > >>> code comment expanded. Do you intend to commit it or do you prefer > >>> that I do? > >> > >> This change looks unrelated: > >> > >> -ERROR: new row for relation "test_jsonb_constraints" violates check > >> constraint "test_jsonb_constraint4" > >> +ERROR: new row for relation "test_jsonb_constraints" violates check > >> constraint "test_jsonb_constraint5" > >> > >> Is this some randomness in the way these constraints are evaluated? > > > > The result of JSON_QUERY() in the CHECK constraint changes, so the > > constraint that previously failed now succeeds after this change, > > because the comparison looked like this before and after: > > > > -- before > > postgres=# select jsonb '[10]' < jsonb '[10]'; > > ?column? > > ---------- > > f > > (1 row) > > > > -- after > > postgres=# select jsonb '10' < jsonb '[10]'; > > ?column? > > ---------- > > t > > (1 row) > > > > That causes the next constraint to be evaluated and its failure > > reported instead. > > > > In the attached, I've adjusted the constraint for the test case to be > > a bit more relevant and removed a nearby somewhat redundant test, > > mainly because its output changes after the adjustment. > > Ok, that looks good. Good that we could clear that up a bit. Thanks for checking. Would you like me to commit it? -- Thanks, Amit Langote
On 11.09.24 13:25, Amit Langote wrote: > On Wed, Sep 11, 2024 at 6:57 PM Peter Eisentraut <peter@eisentraut.org> wrote: >> On 11.09.24 09:51, Amit Langote wrote: >>>>> I've updated your patch to include updated test outputs and a nearby >>>>> code comment expanded. Do you intend to commit it or do you prefer >>>>> that I do? >>>> >>>> This change looks unrelated: >>>> >>>> -ERROR: new row for relation "test_jsonb_constraints" violates check >>>> constraint "test_jsonb_constraint4" >>>> +ERROR: new row for relation "test_jsonb_constraints" violates check >>>> constraint "test_jsonb_constraint5" >>>> >>>> Is this some randomness in the way these constraints are evaluated? >>> >>> The result of JSON_QUERY() in the CHECK constraint changes, so the >>> constraint that previously failed now succeeds after this change, >>> because the comparison looked like this before and after: >>> >>> -- before >>> postgres=# select jsonb '[10]' < jsonb '[10]'; >>> ?column? >>> ---------- >>> f >>> (1 row) >>> >>> -- after >>> postgres=# select jsonb '10' < jsonb '[10]'; >>> ?column? >>> ---------- >>> t >>> (1 row) >>> >>> That causes the next constraint to be evaluated and its failure >>> reported instead. >>> >>> In the attached, I've adjusted the constraint for the test case to be >>> a bit more relevant and removed a nearby somewhat redundant test, >>> mainly because its output changes after the adjustment. >> >> Ok, that looks good. Good that we could clear that up a bit. > > Thanks for checking. Would you like me to commit it? Please do.
On Wed, Sep 11, 2024 at 8:56 PM Peter Eisentraut <peter@eisentraut.org> wrote: > On 11.09.24 13:25, Amit Langote wrote: > > On Wed, Sep 11, 2024 at 6:57 PM Peter Eisentraut <peter@eisentraut.org> wrote: > >> On 11.09.24 09:51, Amit Langote wrote: > >>>>> I've updated your patch to include updated test outputs and a nearby > >>>>> code comment expanded. Do you intend to commit it or do you prefer > >>>>> that I do? > >>>> > >>>> This change looks unrelated: > >>>> > >>>> -ERROR: new row for relation "test_jsonb_constraints" violates check > >>>> constraint "test_jsonb_constraint4" > >>>> +ERROR: new row for relation "test_jsonb_constraints" violates check > >>>> constraint "test_jsonb_constraint5" > >>>> > >>>> Is this some randomness in the way these constraints are evaluated? > >>> > >>> The result of JSON_QUERY() in the CHECK constraint changes, so the > >>> constraint that previously failed now succeeds after this change, > >>> because the comparison looked like this before and after: > >>> > >>> -- before > >>> postgres=# select jsonb '[10]' < jsonb '[10]'; > >>> ?column? > >>> ---------- > >>> f > >>> (1 row) > >>> > >>> -- after > >>> postgres=# select jsonb '10' < jsonb '[10]'; > >>> ?column? > >>> ---------- > >>> t > >>> (1 row) > >>> > >>> That causes the next constraint to be evaluated and its failure > >>> reported instead. > >>> > >>> In the attached, I've adjusted the constraint for the test case to be > >>> a bit more relevant and removed a nearby somewhat redundant test, > >>> mainly because its output changes after the adjustment. > >> > >> Ok, that looks good. Good that we could clear that up a bit. > > > > Thanks for checking. Would you like me to commit it? > > Please do. Done. Thanks for the report and the patch. -- Thanks, Amit Langote
On Thu, Sep 12, 2024 at 8:12 PM Amit Langote <amitlangote09@gmail.com> wrote: > > Hi Andreas, > > On Thu, Sep 12, 2024 at 7:08 PM Andreas Ulbrich > <andreas.ulbrich@matheversum.de> wrote: > > > > Salvete! > > > > > > Sorry for my out of the rules replay, but I'm not at home, and also I can't verify and check your patch. > > > > But I think you have missed the docu in your fix: > > > > Must the example there not also be changed: > > > > From > > > > JSON_QUERY(jsonb '[1,[2,3],null]', 'lax $[*][$off]' PASSING 1 AS off WITH CONDITIONAL WRAPPER) → [3] > > > > to > > > > JSON_QUERY(jsonb '[1,[2,3],null]', 'lax $[*][$off]' PASSING 1 AS off WITH CONDITIONAL WRAPPER) → 3 > > You're right, good catch. > > I had checked whether the documentation text needed fixing, but failed > to notice that an example > is using CONDITIONAL. Will fix, thanks for the report. I have pushed the fix. Thanks Andreas for the report. -- Thanks, Amit Langote