MIME-Version: 1.0
References: <202510210803.3b3vfvygxtjl@alvherre.pgsql>
 <87530674-E6B6-4C97-A704-78C7E07CF01F@gmail.com>
 <3DC56A6A-9525-4C05-B45A-9A135A66BC34@justatheory.com>
 <CACJufxGAmcwq33nr_MmgnGdGo7E10c55kCA22DQ4n9FBUOZmTA@mail.gmail.com>
 <A654130B-16AF-4E37-9D73-B3CDC80C751C@justatheory.com>
In-Reply-To: <A654130B-16AF-4E37-9D73-B3CDC80C751C@justatheory.com>
From: jian he <jian.universality@gmail.com>
Date: Thu, 4 Dec 2025 11:56:31 +0800
Message-ID: 
 <CACJufxEphbmEWLKzYNfzsyEB5+bkbMAMUHJq2LxpDdcSZn98BA@mail.gmail.com>
Subject: Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim,
 replace, split_part
To: "David E. Wheeler" <david@justatheory.com>
Cc: Chao Li <li.evan.chao@gmail.com>,
 =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>,
	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-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CACJufxEphbmEWLKzYNfzsyEB5%2BbkbMAMUHJq2LxpDdcSZn98BA%40mail.gmail.com>
Precedence: bulk

On Mon, Dec 1, 2025 at 5:16=E2=80=AFAM David E. Wheeler <david@justatheory.=
com> wrote:
>
> On Nov 28, 2025, at 05:29, jian he <jian.universality@gmail.com> wrote:
>
> > some "switch" in the attached patch does not preserve the JsonPathItemT=
ype order
> > consistency, like executeItemOptUnwrapTarget.
>
> Well-spotted, thank you! Fixed in v15, attached.
>
hi.

seems no deparse regress tests, like:
create view vj as select jsonb_path_query('"   hello   "', '$.ltrim(" ")') =
as a;
\sv vj

that mean the changes in printJsonPathItem are not tested?


+ /* Create the appropriate jb value to return */
+ switch (jsp->type)
+ {
+ /* Cases for functions that return text */
+ case jpiStrReplace:
comments indentation should align with the word "case"?

executeStringInternalMethod:
+ tmp =3D pnstrdup(jb->val.string.val, jb->val.string.len);
+ str =3D CStringGetTextDatum(tmp);
+
+ /* Internal string functions that accept no arguments */
+ switch (jsp->type)
+ {
+ case jpiStrReplace:
+ {
+ char   *from_str,
+   *to_str;
+ int from_len,
+ to_len;
+
+ jspGetLeftArg(jsp, &elem);
+ 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");
+
+ to_str =3D jspGetString(&elem, &to_len);
+
+ resStr =3D TextDatumGetCString(DirectFunctionCall3Coll(replace_text,
+ DEFAULT_COLLATION_OID,
+ CStringGetTextDatum(tmp),
+ CStringGetTextDatum(from_str),
+ CStringGetTextDatum(to_str)));
+

pnstrdup, CStringGetTextDatum copied twice for the same contend?
I think you can just
``
text *tmp =3D cstring_to_text_with_len(jb->val.string.val, jb->val.string.l=
en);
Datum str =3D PointerGetDatum(tmp)
``
In the first main switch block, there's no need to call
``CStringGetTextDatum(tmp)``
because str is already a Datum. We can simply use str directly.

I noticed that almost all of them use DEFAULT_COLLATION_OID,
but jpiStrSplitPart uses C_COLLATION_OID.


--
jian
https://www.enterprisedb.com