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

On Dec 3, 2025, at 22:56, jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> 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?

I’m afraid I don’t understand. All of the new tests in src/test/regress/sql/jsonpath.sql exercise the printJsonPathItem changes.

> + /* 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"?

Make sense to me; I thought this was indented by pgindent, but I just re-ran it and it didn’t complain.

> 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.

Very close reading, appreciated. I removed tmp altogether, creating this line:

```c
str = PointerGetDatum(cstring_to_text_with_len(jb->val.string.val, jb->val.string.len));
```

And replacing the use of `CStringGetTextDatum(tmp)` in both the `jpiStrReplace` and the `jpiStrSplitPart` cases.

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

Right, fixed.

On Dec 3, 2025, at 23:22, jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> seems no tests for the changes in jspIsMutableWalker too.
> we can make some simple dummy tests like:
>
> create table tjs(a jsonb);
> create index on tjs(jsonb_path_match(a, '$.ltrim(" ")'));

Added to the existing `create index` tests to exercise the new functions.

On Dec 7, 2025, at 06:21, jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> the return value should be
> "def"

Don’t know how that slipped by us; thank you! Fixed by changing the index number to 3.

> The actual signature:
> <replaceable>characters</replaceable> part is optional, but we didn't
> use square brackets
> to indicate it's optional.

Right, fixed.

> similarly:
> string . rtrim([ characters ]) → string
> change to
> ```
> string . rtrim() → string
> string . rtrim(characters ) → string
> ```
>
> string . btrim([ characters ]) → string
> change to
> ```
> string . btrim() → string
> string . btrim([ characters ]) → string
> ``
> would improve the readability, I think.

I can see that, but the syntax here was borrowed from the existing trim functions, which use the [brackets] for the optional args[1]:

<function>rtrim</function> ( <parameter>string</parameter> <type>text</type>
<optional>, <parameter>characters</parameter> <type>text</type> </optional> )
<returnvalue>text</returnvalue>

Updated and rebased patch attached.

Best,

David

[1]: https://github.com/postgres/postgres/blob/ac94ce8/doc/src/sgml/func/func-string.sgml#L383