Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

On May 24, 2025, at 12:51, Florents Tselai <florents(dot)tselai(at)gmail(dot)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 are still 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 that it matters much.

The existing string() method operates on a "JSON boolean, number, string, or datetime"; should these functions also operate on 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 variables for 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 say for sure. And maybe we don't care, consider this a natural extension?

I’ve attached a new patch with docs.

Best,

David