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

Hi Alvora,

I have reviewed and tested the patch, and got a few comments.

> On Oct 21, 2025, at 16:05, Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:
>
> This was lacking rebase after the func.sgml changes. Here it is again.
> I have not reviewed it.
>
> --
> Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
> <v13-0001-Rename-jsonpath-method-arg-tokens.patch><v13-0002-Add-additional-jsonpath-string-methods.patch>

1 - jsonpath.c
```
case jpiStringFunc:
return "string";
+ case jpiStrReplace:
+ return "replace";
+ case jpiStrLower:
+ return "lower";
+ case jpiStrUpper:
+ return "upper";
case jpiTime:
return "time";
case jpiTimeTz:
@@ -914,6 +982,16 @@ jspOperationName(JsonPathItemType type)
return "timestamp";
case jpiTimestampTz:
return "timestamp_tz";
+ case jpiStrLtrim:
+ return "ltrim";
+ case jpiStrRtrim:
+ return "rtrim";
+ case jpiStrBtrim:
+ return "btrim";
+ case jpiStrInitcap:
+ return "initcap";
+ case jpiStrSplitPart:
+ return "split_part";
default:
elog(ERROR, "unrecognized jsonpath item type: %d", type);
return NULL;
```

I wonder if there is some consideration for the order? Feels that jpiSttLtrim and the following jpiStrXXX should be placed above jpiTimeXXX.

2 - jsonpath.c
```
+ if (v->content.arg)
+ {
+ jspGetArg(v, &elem);
+ printJsonPathItem(buf, &elem, false, false);
+ }
```

As there is jspGetArg(), for v->content.arg, does it make sense to add a macro or inline function of jspHasArg(v)?

3 - jsonpath.c
```
+ case jpiStrLtrim:
+ return "ltrim";
+ case jpiStrRtrim:
+ return "rtrim";
+ case jpiStrBtrim:
+ return "btrim";
```

I know “b” in “btrim” stands for “both”, just curious why trim both side function is named “btrim()”? In most of programming languages I am aware of, trim() is the choice.

4 - jsonpath_exec.c
```
default:
;
/* cant' happen */
```

Typo: cant’ -> can’t

5 - jsonpath_exec.c
```
+ /* Create the appropriate jb value to return */
+ switch (jsp->type)
+ {
+ /* Cases for functions that return text */
+ case jpiStrLower:
+ case jpiStrUpper:
+ case jpiStrReplace:
+ case jpiStrLtrim:
+ case jpiStrRtrim:
+ case jpiStrBtrim:
+ case jpiStrInitcap:
+ case jpiStrSplitPart:
+ jb->type = jbvString;
+ jb->val.string.val = resStr;
+ jb->val.string.len = strlen(jb->val.string.val);
+ default:
+ ;
+ /* cant' happen */
+ }
```

As “default” clause has a comment “can’t happen”, I believe “break” is missing in the case clause.

Also, do we want to add an assert in default to report a message in case it happens?

6 - jsonpath_exec.c
```
+ resStr = TextDatumGetCString(DirectFunctionCall3Coll(replace_text,
+ C_COLLATION_OID,
+ CStringGetTextDatum(tmp),
+ CStringGetTextDatum(from_str),
+ CStringGetTextDatum(to_str)));
```

For trim functions, DEFAULT_COLLATION_OID used. Why C_COLLATION_OID is used for replace and split_part? I don’t see anything mentioned in your changes to the doc (func-json.sgml).

7 - jsonpath_exec.c
```
+ if (!(jb = getScalar(jb, jbvString)))
+ RETURN_ERROR(ereport(ERROR,
+ (errcode(ERRCODE_INVALID_ARGUMENT_FOR_SQL_JSON_DATETIME_FUNCTION),
+ errmsg("jsonpath item method .%s() can only be applied to a string",
+ jspOperationName(jsp->type)))));
```

ERRCODE_INVALID_ARGUMENT_FOR_SQL_JSON_DATETIME_FUNCTION seems wrong, this is a string function, not a date time function.

8 - jsonpath_exec.c
```
+ switch (jsp->type)
+ {
+ case jpiStrLtrim:
+ case jpiStrRtrim:
+ case jpiStrBtrim:
+ {
+ char *characters_str;
+ int characters_len;
+ PGFunction func = NULL;
+
+ if (jsp->content.arg)
+ {
+ switch (jsp->type)
+ {
+ case jpiStrLtrim:
+ func = ltrim;
+ break;
+ case jpiStrRtrim:
+ func = rtrim;
+ break;
+ case jpiStrBtrim:
+ func = btrim;
+ break;
+ default:;
+ }
+ jspGetArg(jsp, &elem);
+ if (elem.type != jpiString)
+ elog(ERROR, "invalid jsonpath item type for .%s() argument", jspOperationName(jsp->type));
+
+ characters_str = jspGetString(&elem, &characters_len);
+ resStr = TextDatumGetCString(DirectFunctionCall2Coll(func,
+ DEFAULT_COLLATION_OID, str,
+ CStringGetTextDatum(characters_str)));
+ break;
+ }
+
+ switch (jsp->type)
+ {
+ case jpiStrLtrim:
+ func = ltrim1;
+ break;
+ case jpiStrRtrim:
+ func = rtrim1;
+ break;
+ case jpiStrBtrim:
+ func = btrim1;
+ break;
+ default:;
+ }
+ resStr = TextDatumGetCString(DirectFunctionCall1Coll(func,
+ DEFAULT_COLLATION_OID, str));
+ break;
+ }
```

The two nested “switch (jsp->type)” are quit redundant. We can pull up the second one, and simplify the first one, something like:

```
Switch (jsp->)
{
Case:
..
}

If (jsp->content.arg)
{
jspGetArg(jsp, &elem);
...
}
```

9 - jsonpath_exec.c
```
+ if (elem.type != jpiString)
+ elog(ERROR, "invalid jsonpath item type for .replace() from");
+
+ from_str = jspGetString(&elem, &from_len);
+
+ jspGetRightArg(jsp, &elem);
+ if (elem.type != jpiString)
+ elog(ERROR, "invalid jsonpath item type for .replace() to");
```

In these two elog(), do we want to log the invalid type? As I see in the “default” clause, jsp->type is logged:
```
+ default:
+ elog(ERROR, "unsupported jsonpath item type: %d", jsp->type);
```

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/