Thanks, Peter for the comments. On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut <pe...@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
