Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim,
 replace, split_part
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <202510210803.3b3vfvygxtjl@alvherre.pgsql>
Date: Thu, 23 Oct 2025 10:43:13 +0800
Cc: "David E. Wheeler" <david@justatheory.com>,
 Florents Tselai <florents.tselai@gmail.com>,
 Tom Lane <tgl@sss.pgh.pa.us>,
 Peter Eisentraut <peter@eisentraut.org>,
 Robert Haas <robertmhaas@gmail.com>,
 Alexander Korotkov <aekorotkov@gmail.com>,
 pgsql-hackers <pgsql-hackers@lists.postgresql.org>,
 Andrew Dunstan <andrew@dunslane.net>
Content-Transfer-Encoding: quoted-printable
Message-Id: <87530674-E6B6-4C97-A704-78C7E07CF01F@gmail.com>
References: <202510210803.3b3vfvygxtjl@alvherre.pgsql>
To: =?utf-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>
Archived-At: 
 <https://www.postgresql.org/message-id/87530674-E6B6-4C97-A704-78C7E07CF01F%40gmail.com>
Precedence: bulk

Hi Alvora,

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

> On Oct 21, 2025, at 16:05, =C3=81lvaro Herrera <alvherre@kurilemu.de> =
wrote:
>=20
> This was lacking rebase after the func.sgml changes.  Here it is =
again.
> I have not reviewed it.
>=20
> --=20
> =C3=81lvaro Herrera               48=C2=B001'N 7=C2=B057'E  =E2=80=94  =
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 =E2=80=9Cb=E2=80=9D in =E2=80=9Cbtrim=E2=80=9D stands for =
=E2=80=9Cboth=E2=80=9D, just curious why trim both side function is =
named =E2=80=9Cbtrim()=E2=80=9D? In most of programming languages I am =
aware of, trim() is the choice.=20

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

Typo: cant=E2=80=99 -> can=E2=80=99t=20

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 =3D jbvString;
+			jb->val.string.val =3D resStr;
+			jb->val.string.len =3D =
strlen(jb->val.string.val);
+		default:
+			;
+			/* cant' happen */
+	}
```=20

As =E2=80=9Cdefault=E2=80=9D clause has a comment =E2=80=9Ccan=E2=80=99t =
happen=E2=80=9D, I believe =E2=80=9Cbreak=E2=80=9D 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 =3D =
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=E2=80=99t see anything mentioned =
in your changes to the doc (func-json.sgml).

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

The two nested =E2=80=9Cswitch (jsp->type)=E2=80=9D 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 !=3D jpiString)
+					elog(ERROR, "invalid jsonpath =
item type for .replace() from");
+
+				from_str =3D jspGetString(&elem, =
&from_len);
+
+				jspGetRightArg(jsp, &elem);
+				if (elem.type !=3D 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 =
=E2=80=9Cdefault=E2=80=9D 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/