Re: More new SQL/JSON item methods

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

> Thanks, Peter for the comments.
>
> On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut <peter(at)eisentraut(dot)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