Re: More new SQL/JSON item methods

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