Shouldn't jsonpath .string() Unwrap?

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.

David J.

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() behaves differently 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 2024-06-12 We 16:02, David G. Johnston wrote:

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]https://github.com/theory/postgres/pull/5).

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 fixed it with `git commit --all --amend --date now --reedit-message HEAD` and force-pushed (then fixed the typo and fixed again).

D

Hi,

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

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 covers this issue.

Thanks

On Fri, Jun 14, 2024 at 9:34 PM David E. Wheeler <david@justatheory.com>
wrote:

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 fixed it with `git commit
--all --amend --date now --reedit-message HEAD` and force-pushed (then
fixed the typo and fixed again).

D

--

*Jeevan Chalke*

*Principal, ManagerProduct Development*

enterprisedb.com <https://www.enterprisedb.com&gt;

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 and fixing 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, assuming that it is one of the valid tests. It now returns zero rows instead of an error. Your added test case covers this issue.

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, assuming that it is one of the valid tests. It now returns zero rows instead of an error. Your added test case covers this issue.

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