On May 24, 2025, at 12:51, Florents Tselai <florents.tselai@gmail.com> wrote:
> I think the patch is still in reasonably good shape and hasn’t changed much since September 24.So yes, I’d hope there
arestill some valid points to consider or improve.
Okay, here’s a review.
Patch applies cleanly.
All tests pass.
I'm curious why you added the `arg0` and `arg1` fields to the `method_args` union. Is there some reason that the
existing`left` and `right` fields wouldn't work? Admittedly these are not formally binary operators, but I don't see
thatit matters much.
The existing string() method operates on a "JSON boolean, number, string, or datetime"; should these functions also
operateon all those data types?
The argument to the trim methods appears to be ignored:
```
postgres=# select jsonb_path_query('"zzzytest"', '$.ltrim("xyz")');
jsonb_path_query
------------------
"zzzytest"
```
I'm wondering if the issue is the use of the opt_datetime_template in the grammar?
```
| '.' STR_LTRIM_P '(' opt_datetime_template ')'
{ $$ = makeItemUnary(jpiStrLtrimFunc, $4); }
| '.' STR_RTRIM_P '(' opt_datetime_template ')'
{ $$ = makeItemUnary(jpiStrRtrimFunc, $4); }
| '.' STR_BTRIM_P '(' opt_datetime_template ')'
{ $$ = makeItemUnary(jpiStrBtrimFunc, $4); }
```
I realize it resolves to a string, but for some reason it doesn't get picked up. But also, do you want to support
variablesfor either of these arguments? If so, maybe rename and use starts_with_initial:
```
starts_with_initial:
STRING_P { $$ = makeItemString(&$1); }
| VARIABLE_P { $$ = makeItemVariable(&$1); }
;
```
split_part() does not support a negative n value:
```
postgres=# select split_part('abc,def,ghi,jkl', ',', -2) ;
split_part
------------
ghi
select jsonb_path_query('"abc,def,ghi,jkl"', '$.split_part(",", -2)');
ERROR: syntax error at or near "-" of jsonpath input
LINE 1: select jsonb_path_query('"abc,def,ghi,jkl"', '$.split_part("...
```
Nor does a `+` work. I think you’d be better served using `csv_elem`, something like:
```
| '.' STR_SPLIT_PART_P '(' STRING_P csv_elem ‘)’
```
I'm not sure how well these functions comply with the SQL spec. Does it have a provision for implementation-specific
methods?I *think* all existing methods are defined by the spec, but someone with access to its contents would have to
sayfor sure. And maybe we don't care, consider this a natural extension?
I’ve attached a new patch with docs.
Best,
David
