Skip to content

Add string functions: Trim, LTrim, RTrim, LPad, RPad, Replace#41

Open
seladb wants to merge 5 commits into
tortoise:mainfrom
seladb:add-functions
Open

Add string functions: Trim, LTrim, RTrim, LPad, RPad, Replace#41
seladb wants to merge 5 commits into
tortoise:mainfrom
seladb:add-functions

Conversation

@seladb
Copy link
Copy Markdown

@seladb seladb commented May 13, 2026

  • Add support for trim_chars parameter in the Trim function
  • Add LTrim, RTrim, LPad, RPad, Replace functions (LPad, RPad are not supported in SQLite)

@seladb
Copy link
Copy Markdown
Author

seladb commented May 13, 2026

@abondar @waketzheng @henadzit can you review this PR?

Comment thread tests/test_functions.py
@waketzheng
Copy link
Copy Markdown
Contributor

@seladb Cloud you update the changelog?

It would also be better if you could verify this PR in Tortoise (e.g., https://github.com/tortoise/tortoise-orm/pull/1851/changes).

@seladb
Copy link
Copy Markdown
Author

seladb commented May 25, 2026

@seladb Cloud you update the changelog?

It would also be better if you could verify this PR in Tortoise (e.g., https://github.com/tortoise/tortoise-orm/pull/1851/changes).

@waketzheng I opened this PR in tortoise as WIP: tortoise/tortoise-orm#2202

Comment thread tests/test_functions.py Outdated
Comment thread pypika_tortoise/functions.py Outdated
Comment thread pypika_tortoise/functions.py
@waketzheng
Copy link
Copy Markdown
Contributor

@seladb GPT mentioned that:

Trim, _Pad, and Replace wrap several user-provided values with ValueWrapper(..., allow_parametrize=False), which bypasses get_parameterized_sql() parameterization.

For example, fn.Replace("abcde", "cd", "xx") currently produces SQL like:

("SELECT REPLACE($1,'cd','xx')", ["abcde"])

This is inconsistent with existing function wrappers such as Concat and Substring, where constant function arguments are emitted as query parameters. It also weakens the safety/consistency guarantee callers expect from get_parameterized_sql().

I think these wrappers should keep the default parameterization behavior for trim_chars, length, fill_text, search, and replacement, and add tests for the parameterized SQL output.


Could you please take a look?

@seladb
Copy link
Copy Markdown
Author

seladb commented May 28, 2026

@seladb GPT mentioned that:

Trim, _Pad, and Replace wrap several user-provided values with ValueWrapper(..., allow_parametrize=False), which bypasses get_parameterized_sql() parameterization.

For example, fn.Replace("abcde", "cd", "xx") currently produces SQL like:

("SELECT REPLACE($1,'cd','xx')", ["abcde"])

This is inconsistent with existing function wrappers such as Concat and Substring, where constant function arguments are emitted as query parameters. It also weakens the safety/consistency guarantee callers expect from get_parameterized_sql().

I think these wrappers should keep the default parameterization behavior for trim_chars, length, fill_text, search, and replacement, and add tests for the parameterized SQL output.

Could you please take a look?

I think GPT is correct, I removed allow_parametrize=False. in 28ee5c0

@seladb seladb requested a review from waketzheng May 28, 2026 08:10
@waketzheng
Copy link
Copy Markdown
Contributor

Check this:

Trim.get_function_sql() can bind positional parameters in the wrong order

Trim.get_function_sql() renders non-SQLite SQL as TRIM(BOTH trim_chars FROM term), but parameter values are still collected in the original argument order: term, then trim_chars.

For example, on MySQL-style positional placeholders:

Q.select(fn.Trim("xxabcxx", trim_chars="x")).get_parameterized_sql(MySQLQuery.SQL_CONTEXT)

currently produces:

("SELECT TRIM(BOTH %s FROM %s)", ["xxabcxx", "x"])

The SQL expects the first placeholder to be trim_chars and the second to be term, but the values list is reversed. PostgreSQL may still work because $1/$2 preserve parameter identity, but MySQL/MSSQL/Oracle-style positional placeholders will bind the wrong values.

A fix would be to render/parameterize the arguments in the same order as the SQL text for the non-SQLite branch: self.args[1] first, then self.args[0]. It would also be good to add get_parameterized_sql() coverage for Trim(..., trim_chars=...) on a positional-placeholder dialect such as MySQL or MSSQL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants