Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part - Mailing list pgsql-hackers

Hi Alvora,

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

> On Oct 21, 2025, at 16:05, Álvaro Herrera <alvherre@kurilemu.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
placedabove 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
programminglanguages 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
anythingmentioned 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,
somethinglike: 

```
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/