-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(functions): Make translate function postgres compatible #19630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(functions): Make translate function postgres compatible #19630
Conversation
|
Looks like this affects some existing tests which are now failing; would you be able to double check these against DuckDB & Postgres too? |
|
|
Hi @Jefffrey , I double checked it against DuckDB and Postgres, and the results are the same. |
1fccd03 to
e2c3643
Compare
|
Hi @Jefffrey, is there anything more I have to do here? What is the process to merge it into the main? |
We generally like to leave PRs up for a while after approval in case anyone else wants to take a look or review it. I'll merge it soon. |
Which issue does this PR close?
Rationale for this change
In Postgres
translatefunction implementation, the duplicates in thefromargument are ignored, and the first occurrence wins.A similar implementation is also present in DuckDB.
If the character already exists, the index/mapping is not updated.
https://github.com/duckdb/duckdb/blob/4b7a6b7bd0f8c968bfecab08e801cdc1f0a5cdfd/extension/core_functions/scalar/string/translate.cpp#L45
Before the change in DataFusion
While DuckDB returns
Postgres returns
What changes are included in this PR?
from, the first occurrence wins.fromvalue wasfoowhich contained duplicates and was mappingo->rinstead of Postgress compatibleo->aAre these changes tested?
Are there any user-facing changes?
This is a contract change in some sense. If someone has taken dependency on this behaviour, they will encounter a change. However, I am unsure and need help to properly categorize this.