Обсуждение: Shouldn't jsonpath .string() Unwrap?

Hackers,

Most of the jsonpath methods auto-unwrap in lax mode:

david=# select jsonb_path_query('[-2,5]', '$.abs()');
 jsonb_path_query
------------------
 2
 5
(2 rows)

The obvious exceptions are size() and type(), which apply directly to arrays, so no need to unwrap:

david=# select jsonb_path_query('[-2,5]', '$.size()');
 jsonb_path_query
------------------
 2
(1 row)

david=# select jsonb_path_query('[-2,5]', '$.type()');
 jsonb_path_query
------------------
 "array"

But what about string()? Is there some reason it doesn’t unwrap?

david=# select jsonb_path_query('[-2,5]', '$.string()');
ERROR:  jsonpath item method .string() can only be applied to a bool, string, numeric, or datetime value

What I expect:

david=# select jsonb_path_query('[-2,5]', '$.string()');
 jsonb_path_query
—————————
 "2"
 "5"
(2 rows)

However, I do see a test[1] for this behavior, so maybe there’s a reason for it?

Best,

David

[1]: https://github.com/postgres/postgres/blob/REL_17_BETA1/src/test/regress/expected/jsonb_jsonpath.out#L2527-L2530

On Jun 12, 2024, at 4:02 PM, David G. Johnston <david.g.johnston@gmail.com> wrote:

> Adding Andrew.

Thank you.

> I'm willing to call this an open item against this feature as I don't see any documentation explaining that string()
behavesdifferently than the others. 

Maybe there’s some wording in the standard on this topic?

I’m happy to provide a patch to auto-unwrap .string() in lax mode. Seems pretty straightforward.

D

On Sat, Jun 8, 2024 at 3:50 PM David E. Wheeler <david@justatheory.com> wrote:

Hackers,

Most of the jsonpath methods auto-unwrap in lax mode:

david=# select jsonb_path_query('[-2,5]', '$.abs()');
 jsonb_path_query
------------------
 2
 5
(2 rows)

The obvious exceptions are size() and type(), which apply directly to arrays, so no need to unwrap:

david=# select jsonb_path_query('[-2,5]', '$.size()');
 jsonb_path_query
------------------
 2
(1 row)

david=# select jsonb_path_query('[-2,5]', '$.type()');
 jsonb_path_query
------------------
 "array"

But what about string()? Is there some reason it doesn’t unwrap?

david=# select jsonb_path_query('[-2,5]', '$.string()');
ERROR:  jsonpath item method .string() can only be applied to a bool, string, numeric, or datetime value

What I expect:

david=# select jsonb_path_query('[-2,5]', '$.string()');
 jsonb_path_query
—————————
 "2"
 "5"
(2 rows)

However, I do see a test[1] for this behavior, so maybe there’s a reason for it?

Adding Andrew.

I'm willing to call this an open item against this feature as I don't see any documentation explaining that string() behaves differently than the others.

Hmm. You might be right. Many of these items have this code, but the string() branch does not:

if (unwrap && JsonbType(jb) == jbvArray)
    return executeItemUnwrapTargetArray(cxt, jsp, jb, found,
                                        false);

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Jun 13, 2024, at 3:53 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

> Hmm. You might be right. Many of these items have this code, but the string() branch does not:
> if (unwrap && JsonbType(jb) == jbvArray)
>    return executeItemUnwrapTargetArray(cxt, jsp, jb, found,
>                                        false);

Exactly, would be pretty easy to add. I can work up a patch this weekend.

D

On 06/13/24 18:45, David E. Wheeler wrote:
> On Jun 13, 2024, at 3:53 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>> Hmm. You might be right. Many of these items have this code, but the string() branch does not:
>> if (unwrap && JsonbType(jb) == jbvArray)
>>    return executeItemUnwrapTargetArray(cxt, jsp, jb, found,
>>                                        false);
>
> Exactly, would be pretty easy to add. I can work up a patch this weekend.

My opinion is yes, that should be done. 9.46, umm, General
Rule 11 g ii 6) A) says just "if MODE is lax and <JSON method> is not
type or size, then let BASE be Unwrap(BASE)." No special exemption
there for string(), nor further below at C) XV) for the operation
of string().

Regards,
-Chap

On Jun 13, 2024, at 21:55, Chapman Flack <jcflack@acm.org> wrote:

> My opinion is yes, that should be done. 9.46, umm, General
> Rule 11 g ii 6) A) says just "if MODE is lax and <JSON method> is not
> type or size, then let BASE be Unwrap(BASE)." No special exemption
> there for string(), nor further below at C) XV) for the operation
> of string().

Thank you! Cited that bit in the commit message in the attached patch (also available as a GitHub PR[1]).

D

[1]: https://github.com/theory/postgres/pull/5

On 06/14/24 10:39, David E. Wheeler wrote:
> Cited that bit in the commit message in the attached patch (also available as a GitHub PR[1]).
> 
> [1]: https://github.com/theory/postgres/pull/5

I would s/extepsions/exceptions/ in the added documentation. :)

Offhand (as GitHub PRs aren't really The PG Way), if they were The Way,
I would find this one a little hard to follow, being based at a point
28 unrelated commits ahead of the ref it's opened against. I suspect
'master' on theory/postgres could be fast-forwarded to f1affb6 and then
the PR would look much more approachable.

Regards,
-Chap

> On Jun 14, 2024, at 11:25, Chapman Flack <jcflack@acm.org> wrote:
>
> I would s/extepsions/exceptions/ in the added documentation. :)

Bah, fixed and attached, thanks.

> Offhand (as GitHub PRs aren't really The PG Way), if they were The Way,
> I would find this one a little hard to follow, being based at a point
> 28 unrelated commits ahead of the ref it's opened against. I suspect
> 'master' on theory/postgres could be fast-forwarded to f1affb6 and then
> the PR would look much more approachable.

Yeah, I pushed the PR and branch before I synced master, and GitHub was taking a while to notice and update the PR. I
fixedit with `git commit --all --amend --date now --reedit-message HEAD` and force-pushed (then fixed the typo and
fixedagain). 

D

On Jun 15, 2024, at 10:27, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:

> Sorry, I have missed this in the original patch. I am surprised how that happened. But thanks for catching the same
andfixing it. 

No worries. :-)

> The changes are straightforward and look good to me. However, I have kept the existing test of an empty array as is,
assumingthat it is one of the valid tests. It now returns zero rows instead of an error. Your added test case covers
thisissue. 

Makes sense, thank you.

D

On Jun 15, 2024, at 10:39, David E. Wheeler <david@justatheory.com> wrote:

>> The changes are straightforward and look good to me. However, I have kept the existing test of an empty array as is,
assumingthat it is one of the valid tests. It now returns zero rows instead of an error. Your added test case covers
thisissue. 
>
> Makes sense, thank you.

Added https://commitfest.postgresql.org/48/5039/.

D

On 2024-06-15 Sa 10:51, David E. Wheeler wrote:
> On Jun 15, 2024, at 10:39, David E. Wheeler <david@justatheory.com> wrote:
>
>>> The changes are straightforward and look good to me. However, I have kept the existing test of an empty array as
is,assuming that it is one of the valid tests. It now returns zero rows instead of an error. Your added test case
coversthis issue.
 
>> Makes sense, thank you.
> Added https://commitfest.postgresql.org/48/5039/.
>

Not really needed, I will commit shortly.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Jun 15, 2024, at 12:48, Andrew Dunstan <andrew@dunslane.net> wrote:

> Not really needed, I will commit shortly.

Ah, okay, I wasn’t sure so just defaulted to making sure it was tracked. :-)

Thanks Andrew,

D