More new SQL/JSON item methods

Hi,

On 2023-08-29 03:05, Jeevan Chalke wrote:

This commit implements jsonpath .bigint(), .integer(), and .number()
---
This commit implements jsonpath .date(), .time(), .time_tz(),
.timestamp(), .timestamp_tz() methods.
---
This commit implements jsonpath .boolean() and .string() methods.

Writing as an interested outsider to the jsonpath spec, my first
question would be, is there a published jsonpath spec independent
of PostgreSQL, and are these methods in it, and are the semantics
identical?

The question comes out of my experience on a PostgreSQL integration
of XQuery/XPath, which was nontrivial because the w3 specs for those
languages give rigorous definitions of their data types, independently
of SQL, and a good bit of the work was squinting at those types and at
the corresponding PostgreSQL types to see in what ways they were
different, and what the constraints on converting them were. (Some of
that squinting was already done by the SQL committee in the SQL/XML
spec, which has plural pages on how those conversions have to happen,
especially for the date/time types.)

If I look in [1]https://www.ietf.org/archive/id/draft-goessner-dispatch-jsonpath-00.html, am I looking in the right place for the most
current jsonpath draft?

(I'm a little squeamish reading as a goal "cover only essential
parts of XPath 1.0", given that XPath 1.0 is the one w3 threw away
so XPath 2.0 wouldn't have the same problems.)

On details of the patch itself, I only have quick first impressions,
like:

- surely there's a more direct way to make boolean from numeric
than to serialize the numeric and parse an int?

- I notice that .bigint() and .integer() finish up by casting the
value to numeric so the existing jbv->val.numeric can hold it.
That may leave some opportunity on the table: there is another
patch under way [2]https://commitfest.postgresql.org/44/4476/ that concerns quickly getting such result
values from json operations to the surrounding SQL query. That
could avoid the trip through numeric completely if the query
wants a bigint, if there were a val.bigint in JsonbValue.

But of course that would complicate everything else that
touches JsonbValue. Is there a way for a jsonpath operator to
determine that it's the terminal operation in the path, and
leave a value in val.bigint if it is, or build a numeric if
it's not? Then most other jsonpath code could go on expecting
a numeric value is always in val.numeric, and the only code
checking for a val.bigint would be code involved with
getting the result value out to the SQL caller.

Regards,
-Chap

[1]: https://www.ietf.org/archive/id/draft-goessner-dispatch-jsonpath-00.html
https://www.ietf.org/archive/id/draft-goessner-dispatch-jsonpath-00.html
[2]: https://commitfest.postgresql.org/44/4476/

On 2023-08-30 11:18, Chapman Flack wrote:

If I look in [1], am I looking in the right place for the most
current jsonpath draft?

My bad, I see that it is not. Um if I look in [1'], am I then looking
at the same spec you are?

[1'] https://www.ietf.org/archive/id/draft-ietf-jsonpath-base-20.html

Regards,
-Chap

On 2023-Aug-30, Chapman Flack wrote:

Hi,

On 2023-08-29 03:05, Jeevan Chalke wrote:

This commit implements jsonpath .bigint(), .integer(), and .number()
---
This commit implements jsonpath .date(), .time(), .time_tz(),
.timestamp(), .timestamp_tz() methods.
---
This commit implements jsonpath .boolean() and .string() methods.

Writing as an interested outsider to the jsonpath spec, my first
question would be, is there a published jsonpath spec independent
of PostgreSQL, and are these methods in it, and are the semantics
identical?

Looking at the SQL standard itself, in the 2023 edition section 9.46
"SQL/JSON path language: syntax and semantics", it shows this:

<JSON method> ::=
type <left paren> <right paren>
| size <left paren> <right paren>
| double <left paren> <right paren>
| ceiling <left paren> <right paren>
| floor <left paren> <right paren>
| abs <left paren> <right paren>
| datetime <left paren> [ <JSON datetime template> ] <right paren>
| keyvalue <left paren> <right paren>
| bigint <left paren> <right paren>
| boolean <left paren> <right paren>
| date <left paren> <right paren>
| decimal <left paren> [ <precision> [ <comma> <scale> ] ] <right paren>
| integer <left paren> <right paren>
| number <left paren> <right paren>
| string <left paren> <right paren>
| time <left paren> [ <time precision> ] <right paren>
| time_tz <left paren> [ <time precision> ] <right paren>
| timestamp <left paren> [ <timestamp precision> ] <right paren>
| timestamp_tz <left paren> [ <timestamp precision> ] <right paren>

and then details, for each of those, rules like

III) If JM specifies <double>, then:
1) For all j, 1 (one) ≤ j ≤ n,
Case:
a) If I_j is not a number or character string, then let ST be data
exception — non-numeric SQL/JSON item (22036).
b) Otherwise, let X be an SQL variable whose value is I_j.
Let V_j be the result of
CAST (X AS DOUBLE PRECISION)
If this conversion results in an exception condition, then
let ST be that exception condition.
2) Case:
a) If ST is not successful completion, then the result of JAE
is ST.
b) Otherwise, the result of JAE is the SQL/JSON sequence V_1,
..., V_n.

so at least superficially our implementation is constrained by what the
SQL standard says to do, and we should verify that this implementation
matches those rules. We don't necessarily need to watch what do other
specs such as jsonpath itself.

The question comes out of my experience on a PostgreSQL integration
of XQuery/XPath, which was nontrivial because the w3 specs for those
languages give rigorous definitions of their data types, independently
of SQL, and a good bit of the work was squinting at those types and at
the corresponding PostgreSQL types to see in what ways they were
different, and what the constraints on converting them were.

Yeah, I think the experience of the SQL committee with XML was pretty
bad, as you carefully documented. I hope they don't make such a mess
with JSON.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On 2023-08-30 12:28, Alvaro Herrera wrote:

Yeah, I think the experience of the SQL committee with XML was pretty
bad, as you carefully documented. I hope they don't make such a mess
with JSON.

I guess the SQL committee was taken by surprise after basing something
on Infoset and XPath 1.0 for 2003, and then w3 deciding those things
needed to be scrapped and redone with the lessons learned. So the
SQL committee had to come out with a rather different SQL/XML for 2006,
but I'd say the 2003-2006 difference is the only real 'mess', and other
than going back in time to unpublish 2003, I'm not sure how they'd have
done better.

b) Otherwise, the result of JAE is the SQL/JSON sequence V_1,
..., V_n.

This has my Spidey sense tingling, as it seems very parallel to SQL/XML
where the result of XMLQUERY is to have type XML(SEQUENCE), which is a
type we do not have, and I'm not sure we have a type for "JSON sequence"
either, unless SQL/JSON makes it equivalent to a JSON array (which
I guess is conceivable, more easily than with XML). What does SQL/JSON
say about this SQL/JSON sequence type and how it should behave?

Regards,
-Chap

On 8/30/23 19:20, Chapman Flack wrote:

On 2023-08-30 12:28, Alvaro Herrera wrote:

        b) Otherwise, the result of JAE is the SQL/JSON sequence V_1,
           ..., V_n.

This has my Spidey sense tingling, as it seems very parallel to SQL/XML
where the result of XMLQUERY is to have type XML(SEQUENCE), which is a
type we do not have, and I'm not sure we have a type for "JSON sequence"
either, unless SQL/JSON makes it equivalent to a JSON array (which
I guess is conceivable, more easily than with XML). What does SQL/JSON
say about this SQL/JSON sequence type and how it should behave?

The SQL/JSON data model comprises SQL/JSON items and SQL/JSON sequences.
The components of the SQL/JSON data model are:

— An SQL/JSON item is defined recursively as any of the following:
• An SQL/JSON scalar, defined as a non-null value of any of the
following predefined (SQL) types: character string with character
set Unicode, numeric, Boolean, or datetime.
• An SQL/JSON null, defined as a value that is distinct from any
value of any SQL type. NOTE 109 — An SQL/JSON null is distinct
from the SQL null value.
• An SQL/JSON array, defined as an ordered list of zero or more
SQL/JSON items, called the SQL/JSON elements of the SQL/JSON
array.
• An SQL/JSON object, defined as an unordered collection of zero or
more SQL/JSON members, where an SQL/JSON member is a pair whose
first value is a character string with character set Unicode and
whose second value is an SQL/JSON item. The first value of an
SQL/JSON member is called the key and the second value is called
the bound value.

— An SQL/JSON sequence is an ordered list of zero or more SQL/JSON
items.

--
Vik Fearing

On 2023-08-31 20:50, Vik Fearing wrote:

— An SQL/JSON item is defined recursively as any of the following:
...
• An SQL/JSON array, defined as an ordered list of zero or more
SQL/JSON items, called the SQL/JSON elements of the SQL/JSON
array.
...
— An SQL/JSON sequence is an ordered list of zero or more SQL/JSON
items.

As I was thinking, because "an ordered list of zero or more SQL/JSON
items" is also exactly what an SQL/JSON array is, it seems at least
possible to implement things that are specified to return "SQL/JSON
sequence" by having them return an SQL/JSON array (the kind of thing
that isn't possible for XML(SEQUENCE), because there isn't any other
XML construct that can subsume it).

Still, it seems noteworthy that both terms are used in the spec, rather
than saying the function in question should return a JSON array. Makes
me wonder if there are some other details that make the two distinct.

Regards,
-Chap

Looking at the SQL standard itself, in the 2023 edition section 9.46
"SQL/JSON path language: syntax and semantics", it shows this:

<JSON method> ::=
type <left paren> <right paren>
| size <left paren> <right paren>
| double <left paren> <right paren>
| ceiling <left paren> <right paren>
| floor <left paren> <right paren>
| abs <left paren> <right paren>
| datetime <left paren> [ <JSON datetime template> ] <right paren>
| keyvalue <left paren> <right paren>
| bigint <left paren> <right paren>
| boolean <left paren> <right paren>
| date <left paren> <right paren>
| decimal <left paren> [ <precision> [ <comma> <scale> ] ] <right paren>
| integer <left paren> <right paren>
| number <left paren> <right paren>
| string <left paren> <right paren>
| time <left paren> [ <time precision> ] <right paren>
| time_tz <left paren> [ <time precision> ] <right paren>
| timestamp <left paren> [ <timestamp precision> ] <right paren>
| timestamp_tz <left paren> [ <timestamp precision> ] <right paren>

and then details, for each of those, rules like

III) If JM specifies <double>, then:
1) For all j, 1 (one) ≤ j ≤ n,
Case:
a) If I_j is not a number or character string, then let ST be data
exception — non-numeric SQL/JSON item (22036).
b) Otherwise, let X be an SQL variable whose value is I_j.
Let V_j be the result of
CAST (X AS DOUBLE PRECISION)
If this conversion results in an exception condition, then
let ST be that exception condition.
2) Case:
a) If ST is not successful completion, then the result of JAE
is ST.
b) Otherwise, the result of JAE is the SQL/JSON sequence V_1,
..., V_n.

so at least superficially our implementation is constrained by what the
SQL standard says to do, and we should verify that this implementation
matches those rules. We don't necessarily need to watch what do other
specs such as jsonpath itself.

I believe our current implementation of the .double() method is in line with
this. And these new methods are following the same suit.

- surely there's a more direct way to make boolean from numeric
than to serialize the numeric and parse an int?

Yeah, we can directly check the value = 0 for false, true otherwise.
But looking at the PostgreSQL conversion to bool, it doesn't allow floating
point values to be converted to boolean and only accepts int4. That's why I
did the int4 conversion.

Thanks

--
Jeevan Chalke

*Senior Staff SDE, Database Architect, and ManagerProduct Development*

edbpostgres.com

On 29.08.23 09:05, Jeevan Chalke wrote:

v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch

This commit implements jsonpath .bigint(), .integer(), and .number()
methods.  The JSON string or a numeric value is converted to the
bigint, int4, and numeric type representation.

A comment that applies to all of these: These add various keywords,
switch cases, documentation entries in some order. Are we happy with
that? Should we try to reorder all of that for better maintainability
or readability?

v1-0002-Implement-.date-.time-.time_tz-.timestamp-and-.ti.patch

This commit implements jsonpath .date(), .time(), .time_tz(),
.timestamp(), .timestamp_tz() methods.  The JSON string representing
a valid date/time is converted to the specific date or time type
representation.

The changes use the infrastructure of the .datetime() method and
perform the datatype conversion as appropriate.  All these methods
accept no argument and use ISO datetime formats.

These should accept an optional precision argument. Did you plan to add
that?

v1-0003-Implement-jsonpath-.boolean-and-.string-methods.patch

This commit implements jsonpath .boolean() and .string() methods.

This contains a compiler warning:

../src/backend/utils/adt/jsonpath_exec.c: In function
'executeItemOptUnwrapTarget':
../src/backend/utils/adt/jsonpath_exec.c:1162:86: error: 'tmp' may be
used uninitialized [-Werror=maybe-uninitialized]

v1-0004-Implement-jasonpath-.decimal-precision-scale-meth.patch

This commit implements jsonpath .decimal() method with optional
precision and scale.  If precision and scale are provided, then
it is converted to the equivalent numerictypmod and applied to the
numeric number.

This also contains compiler warnings:

../src/backend/utils/adt/jsonpath_exec.c: In function
'executeItemOptUnwrapTarget':
../src/backend/utils/adt/jsonpath_exec.c:1403:53: error: declaration of
'numstr' shadows a previous local [-Werror=shadow=compatible-local]
../src/backend/utils/adt/jsonpath_exec.c:1442:54: error: declaration of
'elem' shadows a previous local [-Werror=shadow=compatible-local]

There is a typo in the commit message: "Implement jasonpath"

Any reason this patch is separate from 0002? Isn't number() and
decimal() pretty similar?

You could also update src/backend/catalog/sql_features.txt in each patch
(features T865 through T878).

On Fri, Oct 6, 2023 at 7:47 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 29.08.23 09:05, Jeevan Chalke wrote:

v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch

This commit implements jsonpath .bigint(), .integer(), and .number()
methods. The JSON string or a numeric value is converted to the
bigint, int4, and numeric type representation.

A comment that applies to all of these: These add various keywords,
switch cases, documentation entries in some order. Are we happy with
that? Should we try to reorder all of that for better maintainability
or readability?

v1-0002-Implement-.date-.time-.time_tz-.timestamp-and-.ti.patch

This commit implements jsonpath .date(), .time(), .time_tz(),
.timestamp(), .timestamp_tz() methods. The JSON string representing
a valid date/time is converted to the specific date or time type
representation.

The changes use the infrastructure of the .datetime() method and
perform the datatype conversion as appropriate. All these methods
accept no argument and use ISO datetime formats.

These should accept an optional precision argument. Did you plan to add
that?

compiler warnings issue resolved.

I figured out how to use the precision argument.
But I don't know how to get the precision argument in the parse stage.

attached is my attempt to implement: select
jsonb_path_query('"2017-03-10 11:11:01.123"', '$.timestamp(2)');
not that familiar with src/backend/utils/adt/jsonpath_gram.y. imitate
decimal method failed. decimal has precision and scale two arguments.
here only one argument.

looking for hints.

Thanks, Peter for the comments.

On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut <peter@eisentraut.org>
wrote:

On 29.08.23 09:05, Jeevan Chalke wrote:

v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch

This commit implements jsonpath .bigint(), .integer(), and .number()
methods. The JSON string or a numeric value is converted to the
bigint, int4, and numeric type representation.

A comment that applies to all of these: These add various keywords,
switch cases, documentation entries in some order. Are we happy with
that? Should we try to reorder all of that for better maintainability
or readability?

Yeah, that's the better suggestion. While implementing these methods, I was
confused about where to put them exactly and tried keeping them in some
logical place.
I think once these methods get in, we can have a follow-up patch
reorganizing all of these.

v1-0002-Implement-.date-.time-.time_tz-.timestamp-and-.ti.patch

This commit implements jsonpath .date(), .time(), .time_tz(),
.timestamp(), .timestamp_tz() methods. The JSON string representing
a valid date/time is converted to the specific date or time type
representation.

The changes use the infrastructure of the .datetime() method and
perform the datatype conversion as appropriate. All these methods
accept no argument and use ISO datetime formats.

These should accept an optional precision argument. Did you plan to add
that?

Yeah, will add that.

v1-0003-Implement-jsonpath-.boolean-and-.string-methods.patch

This commit implements jsonpath .boolean() and .string() methods.

This contains a compiler warning:

../src/backend/utils/adt/jsonpath_exec.c: In function
'executeItemOptUnwrapTarget':
../src/backend/utils/adt/jsonpath_exec.c:1162:86: error: 'tmp' may be
used uninitialized [-Werror=maybe-uninitialized]

v1-0004-Implement-jasonpath-.decimal-precision-scale-meth.patch

This commit implements jsonpath .decimal() method with optional
precision and scale. If precision and scale are provided, then
it is converted to the equivalent numerictypmod and applied to the
numeric number.

This also contains compiler warnings:

Thanks, for reporting these warnings. I don't get those on my machine, thus
missed them. Will fix them.

../src/backend/utils/adt/jsonpath_exec.c: In function
'executeItemOptUnwrapTarget':
../src/backend/utils/adt/jsonpath_exec.c:1403:53: error: declaration of
'numstr' shadows a previous local [-Werror=shadow=compatible-local]
../src/backend/utils/adt/jsonpath_exec.c:1442:54: error: declaration of
'elem' shadows a previous local [-Werror=shadow=compatible-local]

There is a typo in the commit message: "Implement jasonpath"

Will fix.

Any reason this patch is separate from 0002? Isn't number() and
decimal() pretty similar?

Since DECIMAL has precision and scale arguments, I have implemented that at
the end. I tried merging that with 0001, but other patches ended up with
the conflicts and thus I didn't merge that and kept it as a separate patch.
But yes, logically it belongs to the 0001 group. My bad that I haven't put
in that extra effort. Will do that in the next version. Sorry for the same.

You could also update src/backend/catalog/sql_features.txt in each patch
(features T865 through T878).

OK.

Thanks

--
Jeevan Chalke

*Senior Staff SDE, Database Architect, and ManagerProduct Development*

edbpostgres.com

On Wed, Oct 18, 2023 at 4:50 PM jian he <jian.universality@gmail.com> wrote:

On Fri, Oct 6, 2023 at 7:47 PM Peter Eisentraut <peter@eisentraut.org>
wrote:

On 29.08.23 09:05, Jeevan Chalke wrote:

v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch

This commit implements jsonpath .bigint(), .integer(), and .number()
methods. The JSON string or a numeric value is converted to the
bigint, int4, and numeric type representation.

A comment that applies to all of these: These add various keywords,
switch cases, documentation entries in some order. Are we happy with
that? Should we try to reorder all of that for better maintainability
or readability?

v1-0002-Implement-.date-.time-.time_tz-.timestamp-and-.ti.patch

This commit implements jsonpath .date(), .time(), .time_tz(),
.timestamp(), .timestamp_tz() methods. The JSON string representing
a valid date/time is converted to the specific date or time type
representation.

The changes use the infrastructure of the .datetime() method and
perform the datatype conversion as appropriate. All these methods
accept no argument and use ISO datetime formats.

These should accept an optional precision argument. Did you plan to add
that?

compiler warnings issue resolved.

Thanks for pitching in, Jian.
I was slightly busy with other stuff and thus could not spend time on this.

I will start looking into it and expect a patch in a couple of days.

I figured out how to use the precision argument.
But I don't know how to get the precision argument in the parse stage.

attached is my attempt to implement: select
jsonb_path_query('"2017-03-10 11:11:01.123"', '$.timestamp(2)');
not that familiar with src/backend/utils/adt/jsonpath_gram.y. imitate
decimal method failed. decimal has precision and scale two arguments.
here only one argument.

looking for hints.

You may refer to how .datetime(<format>) is implemented.

Thanks

--
Jeevan Chalke

*Senior Staff SDE, Database Architect, and ManagerProduct Development*

edbpostgres.com

On Thu, Oct 19, 2023 at 11:36 AM Jeevan Chalke <
jeevan.chalke@enterprisedb.com> wrote:

Thanks, Peter for the comments.

On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut <peter@eisentraut.org>
wrote:

On 29.08.23 09:05, Jeevan Chalke wrote:

v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch

This commit implements jsonpath .bigint(), .integer(), and .number()
methods. The JSON string or a numeric value is converted to the
bigint, int4, and numeric type representation.

A comment that applies to all of these: These add various keywords,
switch cases, documentation entries in some order. Are we happy with
that? Should we try to reorder all of that for better maintainability
or readability?

Yeah, that's the better suggestion. While implementing these methods, I
was confused about where to put them exactly and tried keeping them in some
logical place.
I think once these methods get in, we can have a follow-up patch
reorganizing all of these.

v1-0002-Implement-.date-.time-.time_tz-.timestamp-and-.ti.patch

This commit implements jsonpath .date(), .time(), .time_tz(),
.timestamp(), .timestamp_tz() methods. The JSON string representing
a valid date/time is converted to the specific date or time type
representation.

The changes use the infrastructure of the .datetime() method and
perform the datatype conversion as appropriate. All these methods
accept no argument and use ISO datetime formats.

These should accept an optional precision argument. Did you plan to add
that?

Yeah, will add that.

v1-0003-Implement-jsonpath-.boolean-and-.string-methods.patch

This commit implements jsonpath .boolean() and .string() methods.

This contains a compiler warning:

../src/backend/utils/adt/jsonpath_exec.c: In function
'executeItemOptUnwrapTarget':
../src/backend/utils/adt/jsonpath_exec.c:1162:86: error: 'tmp' may be
used uninitialized [-Werror=maybe-uninitialized]

v1-0004-Implement-jasonpath-.decimal-precision-scale-meth.patch

This commit implements jsonpath .decimal() method with optional
precision and scale. If precision and scale are provided, then
it is converted to the equivalent numerictypmod and applied to the
numeric number.

This also contains compiler warnings:

Thanks, for reporting these warnings. I don't get those on my machine,
thus missed them. Will fix them.

../src/backend/utils/adt/jsonpath_exec.c: In function
'executeItemOptUnwrapTarget':
../src/backend/utils/adt/jsonpath_exec.c:1403:53: error: declaration of
'numstr' shadows a previous local [-Werror=shadow=compatible-local]
../src/backend/utils/adt/jsonpath_exec.c:1442:54: error: declaration of
'elem' shadows a previous local [-Werror=shadow=compatible-local]

There is a typo in the commit message: "Implement jasonpath"

Will fix.

Any reason this patch is separate from 0002? Isn't number() and
decimal() pretty similar?

Since DECIMAL has precision and scale arguments, I have implemented that
at the end. I tried merging that with 0001, but other patches ended up with
the conflicts and thus I didn't merge that and kept it as a separate patch.
But yes, logically it belongs to the 0001 group. My bad that I haven't put
in that extra effort. Will do that in the next version. Sorry for the same.

You could also update src/backend/catalog/sql_features.txt in each patch
(features T865 through T878).

OK.

Attached are all three patches fixing the above comments.

Thanks

Thanks

--
Jeevan Chalke

*Senior Staff SDE, Database Architect, and ManagerProduct Development*

edbpostgres.com

--
Jeevan Chalke

*Senior Staff SDE, Database Architect, and ManagerProduct Development*

edbpostgres.com

On Mon, Oct 23, 2023 at 3:29 PM Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:

Attached are all three patches fixing the above comments.

minor issue:
/src/backend/utils/adt/jsonpath_exec.c
2531: Timestamp result;
2532: ErrorSaveContext escontext = {T_ErrorSaveContext};
2533:
2534: /* Get a warning when precision is reduced */
2535: time_precision = anytimestamp_typmod_check(false,
2536: time_precision);
2537: result = DatumGetTimestamp(value);
2538: AdjustTimestampForTypmod(&result, time_precision,
2539: (Node *) &escontext);
2540: if (escontext.error_occurred)
2541: RETURN_ERROR(ereport(ERROR,
2542: (errcode(ERRCODE_INVALID_ARGUMENT_FOR_SQL_JSON_DATETIME_FUNCTION),
2543: errmsg("numeric argument of jsonpath item method .%s() is out
of range for type integer",
2544: jspOperationName(jsp->type)))));

you already did anytimestamp_typmod_check. So this "if
(escontext.error_occurred)" is unnecessary?
A similar case applies to another function called anytimestamp_typmod_check.

/src/backend/utils/adt/jsonpath_exec.c
1493: /* Convert numstr to Numeric with typmod */
1494: Assert(numstr != NULL);
1495: noerr = DirectInputFunctionCallSafe(numeric_in, numstr,
1496: InvalidOid, dtypmod,
1497: (Node *) &escontext,
1498: &numdatum);
1499:
1500: if (!noerr || escontext.error_occurred)
1501: RETURN_ERROR(ereport(ERROR,
1502: (errcode(ERRCODE_NON_NUMERIC_SQL_JSON_ITEM),
1503: errmsg("string argument of jsonpath item method .%s() is not a
valid representation of a decimal or number",
1504: jspOperationName(jsp->type)))));

inside DirectInputFunctionCallSafe already "if (SOFT_ERROR_OCCURRED(escontext))"
so "if (!noerr || escontext.error_occurred)" change to "if (!noerr)"
should be fine?

On 2023-10-19 Th 02:06, Jeevan Chalke wrote:

Thanks, Peter for the comments.

On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut <peter@eisentraut.org>
wrote:

On 29.08.23 09:05, Jeevan Chalke wrote:

v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch

This commit implements jsonpath .bigint(), .integer(), and .number()
methods.  The JSON string or a numeric value is converted to the
bigint, int4, and numeric type representation.

A comment that applies to all of these: These add various keywords,
switch cases, documentation entries in some order.  Are we happy with
that?  Should we try to reorder all of that for better
maintainability
or readability?

Yeah, that's the better suggestion. While implementing these methods,
I was confused about where to put them exactly and tried keeping them
in some logical place.
I think once these methods get in, we can have a follow-up patch
reorganizing all of these.

I think it would be better to organize things how we want them before
adding in more stuff.

cheers

andrew

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

Hello,

On Tue, Oct 24, 2023 at 6:41 PM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2023-10-19 Th 02:06, Jeevan Chalke wrote:

Thanks, Peter for the comments.

On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut <peter@eisentraut.org>
wrote:

On 29.08.23 09:05, Jeevan Chalke wrote:

v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch

This commit implements jsonpath .bigint(), .integer(), and .number()
methods. The JSON string or a numeric value is converted to the
bigint, int4, and numeric type representation.

A comment that applies to all of these: These add various keywords,
switch cases, documentation entries in some order. Are we happy with
that? Should we try to reorder all of that for better maintainability
or readability?

Yeah, that's the better suggestion. While implementing these methods, I
was confused about where to put them exactly and tried keeping them in some
logical place.
I think once these methods get in, we can have a follow-up patch
reorganizing all of these.

I think it would be better to organize things how we want them before
adding in more stuff.

I have tried reordering all the jsonpath Operators and Methods
consistently. With this patch, they all appear in the same order when
together in the group.

In some switch cases, they are still divided, like in
flattenJsonPathParseItem(), where 2-arg, 1-arg, and no-arg cases are
clubbed together. But I have tried to keep them in order in those subgroups.

I will rebase my patches for this task on this patch, but before doing so,
I would like to get your views on this reordering.

Thanks

cheers

andrew

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

--
Jeevan Chalke

*Senior Staff SDE, Database Architect, and ManagerProduct Development*

edbpostgres.com

On 2023-11-01 We 03:00, Jeevan Chalke wrote:

Hello,

On Tue, Oct 24, 2023 at 6:41 PM Andrew Dunstan <andrew@dunslane.net>
wrote:

On 2023-10-19 Th 02:06, Jeevan Chalke wrote:

Thanks, Peter for the comments.

On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut
<peter@eisentraut.org> wrote:

On 29.08.23 09:05, Jeevan Chalke wrote:

v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch

This commit implements jsonpath .bigint(), .integer(), and

.number()

methods.  The JSON string or a numeric value is converted

to the

bigint, int4, and numeric type representation.

A comment that applies to all of these: These add various
keywords,
switch cases, documentation entries in some order.  Are we
happy with
that?  Should we try to reorder all of that for better
maintainability
or readability?

Yeah, that's the better suggestion. While implementing these
methods, I was confused about where to put them exactly and tried
keeping them in some logical place.
I think once these methods get in, we can have a follow-up patch
reorganizing all of these.

I think it would be better to organize things how we want them
before adding in more stuff.

I have tried reordering all the jsonpath Operators and Methods
consistently. With this patch, they all appear in the same order when
together in the group.

In some switch cases, they are still divided, like in
flattenJsonPathParseItem(), where 2-arg, 1-arg, and no-arg cases are
clubbed together. But I have tried to keep them in order in those
subgroups.

I will rebase my patches for this task on this patch, but before doing
so, I  would like to get your views on this reordering.

This appears to be reasonable. Maybe we need to add a note in one or two
places about maintaining the consistency?

cheers

andrew

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

On Wed, Nov 1, 2023 at 3:49 PM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2023-11-01 We 03:00, Jeevan Chalke wrote:

Hello,

On Tue, Oct 24, 2023 at 6:41 PM Andrew Dunstan <andrew@dunslane.net>
wrote:

On 2023-10-19 Th 02:06, Jeevan Chalke wrote:

Thanks, Peter for the comments.

On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut <peter@eisentraut.org>
wrote:

On 29.08.23 09:05, Jeevan Chalke wrote:

v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch

This commit implements jsonpath .bigint(), .integer(), and .number()
methods. The JSON string or a numeric value is converted to the
bigint, int4, and numeric type representation.

A comment that applies to all of these: These add various keywords,
switch cases, documentation entries in some order. Are we happy with
that? Should we try to reorder all of that for better maintainability
or readability?

Yeah, that's the better suggestion. While implementing these methods, I
was confused about where to put them exactly and tried keeping them in some
logical place.
I think once these methods get in, we can have a follow-up patch
reorganizing all of these.

I think it would be better to organize things how we want them before
adding in more stuff.

I have tried reordering all the jsonpath Operators and Methods
consistently. With this patch, they all appear in the same order when
together in the group.

In some switch cases, they are still divided, like in
flattenJsonPathParseItem(), where 2-arg, 1-arg, and no-arg cases are
clubbed together. But I have tried to keep them in order in those subgroups.

I will rebase my patches for this task on this patch, but before doing so,
I would like to get your views on this reordering.

This appears to be reasonable. Maybe we need to add a note in one or two
places about maintaining the consistency?

+1
Added a note in jsonpath.h where enums are defined.

I have rebased all three patches over this reordering patch making 4
patches in the set.

Let me know your views on the same.

Thanks

cheers

andrew

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

--
Jeevan Chalke

*Senior Staff SDE, Database Architect, and ManagerProduct Development*

edbpostgres.com

On 2023-11-06 Mo 08:23, Jeevan Chalke wrote:

On Wed, Nov 1, 2023 at 3:49 PM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2023-11-01 We 03:00, Jeevan Chalke wrote:

Hello,

On Tue, Oct 24, 2023 at 6:41 PM Andrew Dunstan
<andrew@dunslane.net> wrote:

On 2023-10-19 Th 02:06, Jeevan Chalke wrote:

Thanks, Peter for the comments.

On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut
<peter@eisentraut.org> wrote:

On 29.08.23 09:05, Jeevan Chalke wrote:

v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch

This commit implements jsonpath .bigint(), .integer(),

and .number()

methods.  The JSON string or a numeric value is

converted to the

bigint, int4, and numeric type representation.

A comment that applies to all of these: These add
various keywords,
switch cases, documentation entries in some order.  Are
we happy with
that?  Should we try to reorder all of that for better
maintainability
or readability?

Yeah, that's the better suggestion. While implementing these
methods, I was confused about where to put them exactly and
tried keeping them in some logical place.
I think once these methods get in, we can have a follow-up
patch reorganizing all of these.

I think it would be better to organize things how we want
them before adding in more stuff.

I have tried reordering all the jsonpath Operators and Methods
consistently. With this patch, they all appear in the same order
when together in the group.

In some switch cases, they are still divided, like in
flattenJsonPathParseItem(), where 2-arg, 1-arg, and no-arg cases
are clubbed together. But I have tried to keep them in order in
those subgroups.

I will rebase my patches for this task on this patch, but before
doing so, I  would like to get your views on this reordering.

This appears to be reasonable. Maybe we need to add a note in one
or two places about maintaining the consistency?

+1
Added a note in jsonpath.h where enums are defined.

I have rebased all three patches over this reordering patch making 4
patches in the set.

Let me know your views on the same.

Thanks

Hi Jeevan,

I think these are in reasonably good shape, but there are a few things
that concern me:

andrew@~=# select jsonb_path_query_array('[1.2]', '$[*].bigint()');
ERROR:  numeric argument of jsonpath item method .bigint() is out of
range for type bigint

I'm ok with this being an error, but I think the error message is wrong.
It should be the "invalid input" message.

andrew@~=# select jsonb_path_query_array('[1.0]', '$[*].bigint()');
ERROR:  numeric argument of jsonpath item method .bigint() is out of
range for type bigint

Should we trim trailing dot+zeros from numeric values before trying to
convert to bigint/int? If not, this too should be an "invalid input" case.

andrew@~=# select jsonb_path_query_array('[1.0]', '$[*].boolean()');
ERROR:  numeric argument of jsonpath item method .boolean() is out of
range for type boolean

It seems odd that any non-zero integer is true but not any non-zero
numeric. Is that in the spec? If not I'd avoid trying to convert it to
an integer first, and just check for infinity/nan before looking to see
if it's zero.

The code for integer() and bigint() seems a bit duplicative, but I'm not
sure there's a clean way of avoiding that.

The items for datetime types and string look OK.

cheers

andrew

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

On Sun, Dec 3, 2023 at 9:44 PM Andrew Dunstan <andrew@dunslane.net> wrote:

Hi Jeevan,

I think these are in reasonably good shape, but there are a few things
that concern me:

andrew@~=# select jsonb_path_query_array('[1.2]', '$[*].bigint()');
ERROR: numeric argument of jsonpath item method .bigint() is out of range
for type bigint

I'm ok with this being an error, but I think the error message is wrong.
It should be the "invalid input" message.

andrew@~=# select jsonb_path_query_array('[1.0]', '$[*].bigint()');
ERROR: numeric argument of jsonpath item method .bigint() is out of range
for type bigint

Should we trim trailing dot+zeros from numeric values before trying to
convert to bigint/int? If not, this too should be an "invalid input" case.

We have the same issue with integer conversion and need a fix.

Unfortunately, I was using int8in() for the conversion of numeric values.
We should be using numeric_int8() instead. However, there is no opt_error
version of the same.

So, I have introduced a numeric_int8_opt_error() version just like we have
one for int4, i.e. numeric_int4_opt_error(), to suppress the error. These
changes are in the 0001 patch. (All other patch numbers are now increased
by 1)

I have used this new function to fix this reported issue and used
numeric_int4_opt_error() for integer conversion.

andrew@~=# select jsonb_path_query_array('[1.0]', '$[*].boolean()');
ERROR: numeric argument of jsonpath item method .boolean() is out of
range for type boolean

It seems odd that any non-zero integer is true but not any non-zero
numeric. Is that in the spec? If not I'd avoid trying to convert it to an
integer first, and just check for infinity/nan before looking to see if
it's zero.

PostgreSQL doesn’t cast a numeric to boolean. So maybe we should keep this
behavior as is.

# select 1.0::boolean;
ERROR: cannot cast type numeric to boolean
LINE 1: select 1.0::boolean;

The code for integer() and bigint() seems a bit duplicative, but I'm not
sure there's a clean way of avoiding that.

The items for datetime types and string look OK.

Thanks.

Suggestions?

cheers

andrew

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

--
Jeevan Chalke

*Senior Staff SDE, Database Architect, and ManagerProduct Development*

edbpostgres.com