public inbox for [email protected]  

help / color / mirror / Atom feed

From: jian he <[email protected]>
To: David E. Wheeler <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Florents Tselai <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Subject: Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part
Date: Thu, 4 Dec 2025 11:56:31 +0800
Message-ID: <CACJufxEphbmEWLKzYNfzsyEB5+bkbMAMUHJq2LxpDdcSZn98BA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<CACJufxGAmcwq33nr_MmgnGdGo7E10c55kCA22DQ4n9FBUOZmTA@mail.gmail.com>
	<[email protected]>

On Mon, Dec 1, 2025 at 5:16 AM David E. Wheeler <[email protected]> wrote:
>
> On Nov 28, 2025, at 05:29, jian he <[email protected]> wrote:
>
> > some "switch" in the attached patch does not preserve the JsonPathItemType 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 = pnstrdup(jb->val.string.val, jb->val.string.len);
+ str = 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 != 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");
+
+ to_str = jspGetString(&elem, &to_len);
+
+ resStr = 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 = cstring_to_text_with_len(jb->val.string.val, jb->val.string.len);
Datum str = 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

view thread (51+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part
  In-Reply-To: <CACJufxEphbmEWLKzYNfzsyEB5+bkbMAMUHJq2LxpDdcSZn98BA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox