Hi Alvora, I have reviewed and tested the patch, and got a few comments.
> On Oct 21, 2025, at 16:05, Álvaro Herrera <[email protected]> wrote: > > This was lacking rebase after the func.sgml changes. Here it is again. > I have not reviewed it. > > -- > Álvaro Herrera 48°01'N 7°57'E — 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 “b” in “btrim” stands for “both”, just curious why trim both side function is named “btrim()”? In most of programming languages I am aware of, trim() is the choice. 4 - jsonpath_exec.c ``` default: ; /* cant' happen */ ``` Typo: cant’ -> can’t 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 = jbvString; + jb->val.string.val = resStr; + jb->val.string.len = strlen(jb->val.string.val); + default: + ; + /* cant' happen */ + } ``` As “default” clause has a comment “can’t happen”, I believe “break” 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 = 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’t see anything mentioned in your changes to the doc (func-json.sgml). 7 - jsonpath_exec.c ``` + if (!(jb = 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 = NULL; + + if (jsp->content.arg) + { + switch (jsp->type) + { + case jpiStrLtrim: + func = ltrim; + break; + case jpiStrRtrim: + func = rtrim; + break; + case jpiStrBtrim: + func = btrim; + break; + default:; + } + jspGetArg(jsp, &elem); + if (elem.type != jpiString) + elog(ERROR, "invalid jsonpath item type for .%s() argument", jspOperationName(jsp->type)); + + characters_str = jspGetString(&elem, &characters_len); + resStr = TextDatumGetCString(DirectFunctionCall2Coll(func, + DEFAULT_COLLATION_OID, str, + CStringGetTextDatum(characters_str))); + break; + } + + switch (jsp->type) + { + case jpiStrLtrim: + func = ltrim1; + break; + case jpiStrRtrim: + func = rtrim1; + break; + case jpiStrBtrim: + func = btrim1; + break; + default:; + } + resStr = TextDatumGetCString(DirectFunctionCall1Coll(func, + DEFAULT_COLLATION_OID, str)); + break; + } ``` The two nested “switch (jsp->type)” 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 != 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"); ``` In these two elog(), do we want to log the invalid type? As I see in the “default” 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/
