json_query conditional wrapper bug

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.

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

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

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?

--
Thanks, Amit Langote

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 Wed, Sep 11, 2024 at 5:15 AM Peter Eisentraut <peter@eisentraut.org> wrote:

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?

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.

--
Thanks, Amit Langote

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