On Dec 3, 2025, at 22:56, jian he <jian.universality@gmail.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.universality@gmail.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.universality@gmail.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
optionalargs[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
