On May 24, 2025, at 12:51, Florents Tselai <florents.tse...@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 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
v3-0001-Add-additional-jsonpath-string-methods.patch
Description: Binary data
signature.asc
Description: Message signed with OpenPGP
