Feedback-ID: ie0a040ee:Fastmail
Message-ID: <6d4feb29-6ced-a7b0-2c15-42d3e8e201f7@eisentraut.org>
Date: Fri, 6 Oct 2023 13:43:44 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.15.1
Subject: Re: More new SQL/JSON item methods
Content-Language: en-US
To: Jeevan Chalke <jeevan.chalke@enterprisedb.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <CAM2+6=XjTyqrrqHAOj80r0wVQxJSxc0iyib9bPC55uFO9VKatg@mail.gmail.com>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <CAM2+6=XjTyqrrqHAOj80r0wVQxJSxc0iyib9bPC55uFO9VKatg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/6d4feb29-6ced-a7b0-2c15-42d3e8e201f7%40eisentraut.org>
Precedence: bulk

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